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() }
>>>>>>>>>> }
>>>>>>>>>> }
>>>>>>>>>> }
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>
>>>>>>
>>>
>>>
>>
>
