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