OK, I think nobody else cares because you indeed rarely run several OFBiz
instances on the same machine, apart in demos and maybe in development.
So I'll create a script specific to demos where I need to sometimes kill only
one of the 3 instances and not all.
Jacques
Le 27/06/2017 à 15:12, Taher Alkhateeb a écrit :
I think terminateOfbiz should NOT be portoffset-aware nor should it
kill only one instance. terminateOfbiz is a convenience task to kill
all instances which is mostly used during development. That's the only
purpose it has. I believe it should NOT be used in any automation
suite or buildbot or anything like that.
I have already spent a lot of time correcting your errors in that task
[1]. Please let's try to avoid repeating this time consuming process.
[1] https://s.apache.org/fvBB
On Tue, Jun 27, 2017 at 3:39 PM, Jacques Le Roux
<jacques.le.r...@les7arts.com> wrote:
We still have a problem with terminateOfbiz: it's not portoffset aware.
So when you run it, it closes all OFBiz instances running on a machine (eg
on VM demo)
I guess we only need to improve terminateOfbiz by passing a portoffset
parameter
I'll create a Jira and provide a patch for review
Jacques
Le 26/06/2017 à 09:37, Jacques Le Roux a écrit :
OK, the shutdown command is somehow our SIGTERM, makes sense
Jacques
Le 26/06/2017 à 08:45, Taher Alkhateeb a écrit :
I do not think we should send SIGTERM in the first place. The proper
termination of OFBiz is with the --shutdown command which cleanly stops
everything and calls the shutdown hook. The gradle task "terminateOfbiz"
should only be used if the shutdown command fails (last resort) as cleary
mentioned in the docs.
On Jun 26, 2017 9:19 AM, "Jacques Le Roux" <jacques.le.r...@les7arts.com>
wrote:
Thanks Taher,
I indeed missed that kill is not an atomic operation. I reverted at
r1799852.
Before reading your last message I wanted to propose to set a delay
between
the 2 operations.
So SIGTERM fails. I'd really like to send SIGTERM before killing the
process and only kill it if it's necessary (ie if SIGTERM did not
terminate
the process)
So we need to catch the SIGTERM exec and send SIGKILL if it fails, right?
Jacques
Le 25/06/2017 à 16:05, Taher Alkhateeb a écrit :
Oh and to prove the second scenario, just add a sleep for a few seconds
after the SIGTERM kill command and observe how the system will crash:
if (line ==~ /.*org\.apache\.ofbiz\.base\.start\.Start.*/) {
exec { commandLine 'kill', '-TERM', line.tokenize().first() }
Thread.sleep(5000)
}
Again, this shows you how poor the quality of this code is. It
introduces a
subtle bug, it repeats an if condition unnecessarily, and the comments
are
wrong "Only kill if needed"
On Sun, Jun 25, 2017 at 4:22 PM, Taher Alkhateeb <
slidingfilame...@gmail.com
wrote:
I'm completing my answer for the record to show exactly where your code
is
faulty and wrong just like in the other previous cases.
1- you have a copy-paste pattern in your code
2- you are calling the kill command twice immediately one after another
without checking whether SIGTERM killed the process properly. The whole
purpose of doing SIGTERM is to allow cleaning resources before
enforcing
SIGKILL. You're not giving the JVM an opportunity to call the shutdown
hook
because you're looking up the same output from the same "ps ax" command
for
both kill commands.
So your code is working almost exactly as if typing:
if (line ==~ /.*org\.apache\.ofbiz\.base\.start\.Start.*/) {
exec { commandLine 'kill', '-TERM', line.tokenize().first() }
exec { commandLine 'kill', '-KILL', line.tokenize().first() }
}
Now what's happening is that SIGKILL is executing before SIGTERM
completes, so the build is successful but OFBiz is killed with SIGKILL
not
SIGTERM. If SIGTERM finishes very quickly (does not happen with me)
then
the build system would crash because the exit value of SIGKILL would be
1,
not 0 (process not found). So either way it is a problem (either
killing
with SIGKILL or crashing the build system)
Convinced? I hope you can revert now.
On Sun, Jun 25, 2017 at 2:20 PM, Jacques Le Roux <
jacques.le.r...@les7arts.com> wrote:
But what's wrong? It's not clear to me, do you see it Michael?
Not clearly answering this question just adds more work on my side, I
still don't see why I should revert.
Jacques
Le 25/06/2017 à 13:11, Michael Brohl a écrit :
I's like to propose the following process to get this worked out
without
endless ping-pong here on the mailing lists:
1. revert the change as requested
2. provide a Jira with patch and review/discuss it there (what's the
problem, how should it be solved etc.)
3. come to a conclusion and a thumbsup by other committers
4. commit the solution.
Thanks,
Michael
Am 25.06.17 um 12:36 schrieb Taher Alkhateeb:
OK, I take this question as your refusal to revert and cooperate. I'm
done
spending time here and I leave it up to the community to see if they
want
incorrect code in the code base.
On Sun, Jun 25, 2017 at 1:34 PM, Jacques Le Roux <
jacques.le.r...@les7arts.com> wrote:
Mmm but anyway, I remember now how I thought that.
In the 2nd "if", if the process was terminated, kill will just say
that it
can't find the process and exec will return. Else the process will
be
killed.
What's wrong then?
Jacques
Le 25/06/2017 à 12:28, Jacques Le Roux a écrit :
Ah wait, I see line still contains "start" so will be executed
twice
anyway. OK I'll improve that.
Jacques
Le 25/06/2017 à 12:18, Jacques Le Roux a écrit :
I don't revert without explanations on why I should revert. Sorry,
but I
don't find your explanations clear.
My explanation, tell my what's wrong:
The 2 "if" blocks are executed sequentially for each line
containing
"a
start"and ignore other lines. I did not change the previous
logic,
just
added a new if.
Gradle exec spawns a new process and waits till it ends (this
point
is
important).
If the line contains "a start" the 1st "if" try to terminate it.
If it worked the 2nd "if" does not execute. This is better than
before
because it allows the "'start" process "a chance to clean up
after
itself"
(cf unix.stackexchange.com below)
If the "start" process is not terminated then it's killed by the
2nd
"if", like it was done before.
As I said it cleanly worked with 2 OFBiz instances running in the
background.
Now tell me what's wrong?
Thanks
Jacques
Le 25/06/2017 à 11:22, Taher Alkhateeb a écrit :
As usual, you refuse to revert because you don't understand the
code
and I
pay the price of spending my time explaining. I hope you will
reconsider
this time consuming and non-cooperative behavior.
The quick version:
- copy and paste antipattern
- incorrect conditional checking leading to both blocks getting
executed or
both blocks not executing
Your belief that Gradle fails because java does not expect to be
killed
is
amazing! It means you do not understand what this code is doing
and
what is
causing the failure.
On Jun 25, 2017 10:42 AM, "Jacques Le Roux" <
jacques.le.r...@les7arts.com>
wrote:
What makes you think it's wrong? I tested it locally using 2
background
instances and it cleaned worked.
I also tried with one standard instance (not in background). It
works,
and
you get this message
:ofbiz FAILED
FAILURE: Build failed with an exception.
* What went wrong:
Execution failed for task ':ofbiz'.
Process 'command '/usr/lib/jvm/java-8-oracle/bin/java'' finished
with
non-zero exit value 137
Which I believe is OK because Java does not expect to be killed!
Jacques
Le 24/06/2017 à 20:04, Taher Alkhateeb a écrit :
This commit is wrong and bad on multiple levels. Please revert
On Sat, Jun 24, 2017 at 10:56 AM, <jler...@apache.org> wrote:
Author: jleroux
Date: Sat Jun 24 07:56:45 2017
New Revision: 1799736
URL: http://svn.apache.org/viewvc?rev=1799736&view=rev
Log:
No functional change
Improves terminateOfbiz byt using TERM before KILL
https://fr.wikipedia.org/wiki/Kill_(Unix)
https://unix.stackexchange.com/questions/8916/when-
should-i-not-kill-9-a-process
Modified:
ofbiz/ofbiz-framework/trunk/build.gradle
Modified: ofbiz/ofbiz-framework/trunk/build.gradle
URL: http://svn.apache.org/viewvc/ofbiz/ofbiz-framework/trunk/
build.gradle?rev=1799736&r1=1799735&r2=1799736&view=diff
============================================================
==================
--- ofbiz/ofbiz-framework/trunk/build.gradle (original)
+++ ofbiz/ofbiz-framework/trunk/build.gradle Sat Jun 24
07:56:45
2017
@@ -332,8 +332,13 @@ task terminateOfbiz(group: ofbizServer,
standardOutput = processOutput
}
processOutput.toString().split(System.lineSeparator()).each
{ line ->
+ // Try to terminate cleanly
if (line ==~
/.*org\.apache\.ofbiz\.base\.s
tart\.Start.*/)
{
- exec { commandLine 'kill', '-9',
line.tokenize().first() }
+ exec { commandLine 'kill', '-TERM',
line.tokenize().first() }
+ }
+ // Only kill if needed
+ if (line ==~ /.*org\.apache\.ofbiz\.base\.s
tart\.Start.*/)
{
+ exec { commandLine 'kill', '-KILL',
line.tokenize().first() }
}
}
}
