You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@ofbiz.apache.org by Jacques Le Roux <ja...@les7arts.com> on 2017/06/27 12:39:29 UTC
A problem with terminateOfbiz Gradle task (was [Re: svn commit:
r1799736 - /ofbiz/ofbiz-framework/trunk/build.gradle)
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" <ja...@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 <
>>> slidingfilaments@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.roux@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.roux@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.roux@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, <jl...@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() }
>>>>>>>>>>>>> }
>>>>>>>>>>>>> }
>>>>>>>>>>>>> }
>>>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>>>
>
>
Re: A problem with terminateOfbiz Gradle task (was [Re: svn commit:
r1799736 - /ofbiz/ofbiz-framework/trunk/build.gradle)
Posted by Jacques Le Roux <ja...@les7arts.com>.
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
> <ja...@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" <ja...@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 <
>>>>> slidingfilaments@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.roux@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.roux@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.roux@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, <jl...@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() }
>>>>>>>>>>>>>>> }
>>>>>>>>>>>>>>> }
>>>>>>>>>>>>>>> }
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>
>>>
Re: A problem with terminateOfbiz Gradle task (was [Re: svn commit:
r1799736 - /ofbiz/ofbiz-framework/trunk/build.gradle)
Posted by Taher Alkhateeb <sl...@gmail.com>.
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
<ja...@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" <ja...@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 <
>>>> slidingfilaments@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.roux@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.roux@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.roux@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, <jl...@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() }
>>>>>>>>>>>>>> }
>>>>>>>>>>>>>> }
>>>>>>>>>>>>>> }
>>>>>>>>>>>>>>
>>>>>>>>>>>>>>
>>>>>>>>>>>>>>
>>>>>>>>>>>>>>
>>>>>>>>>>>>>>
>>>>>>>>>>>>>>
>>>>>>>>>>>>>>
>>
>>
>