[#305] updates pre-push script to immediately kill all child processes

[thesheps]

Fixes issue #305

I think the reason this was failing originally is because child processes such as node weren't being immediately killed. This is a bit of a nuclear option admittedly, but I'm pretty sure pgrep is a standard linux tool these days?

@build-canaries/owners

[GentlemanHal]

Ah, so the problem I've seen isn't the develop script not getting stopped. It's actually the processes launched by the pre-push (server and the mock server) script not getting stopped sometimes.

I'm fairly sure it only happens when the journey tests fail, I presume because line https://github.com/build-canaries/nevergreen/blob/main/pre-push.sh#L61 is returning a non-zero exit code which kills the script.

I also assumed line https://github.com/build-canaries/nevergreen/blob/main/pre-push.sh#L47 should still be "catching" the termination of the script and killing all the child process (those started on line 49 and 52) before exiting?!

I really have very little idea about bash scripts and hacked these together with help from the internet! So it's very likely my assumptions about how this is all working is incorrect.

[thesheps]

That's exactly right, yep! Although I'm able to reproduce the issue reliably by:

• running develop.sh
• immediately running pre-push.sh

Whilst the node process is doing it's thing (or nvm or whatever), you can see the child processes fail to stop :)

On Thu, Jan 12, 2023, 23:50 Christopher Martin @.***> wrote:

Ah, so the problem I've seen isn't the develop script not getting stopped. It's actually the processes launched by the pre-push (server and the mock server) script not getting stopped sometimes.

I'm fairly sure it only happens when the journey tests fail, I presume because line https://github.com/build-canaries/nevergreen/blob/main/pre-push.sh#L61 is returning a non-zero exit code which kills the script.

I also assumed line https://github.com/build-canaries/nevergreen/blob/main/pre-push.sh#L47 should still be "catching" the termination of the script and killing all the child process before exiting?!

I really have very little idea about bash scripts and hacked these together with help from the internet! So it's very likely my assumptions about how this is all working is incorrect.

— Reply to this email directly, view it on GitHub https://github.com/build-canaries/nevergreen/pull/351#issuecomment-1381115544, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAWSTN6COTUTPJUYPIPECZDWSCKFVANCNFSM6AAAAAATZWBXIE . You are receiving this because you authored the thread.Message ID: @.***>

[GentlemanHal]

Seems like we have 2 different but similar issues.

1. pre-push not being able to stop develop cleanly (what you've found)
2. pre-push not cleaning up after itself correctly (what I found)

I realised while writing this that I always manually stop develop before running pre-push so that explains why I never noticed the issue you've found.

If this was a pure node project we'd be making better use of npm commands and tools like concurrently. We didn't originally as it's a mixed repo and the client used to be quite simple. Now the client is much more complex and most features are client side nowadays.

So treating it like a node project and doing more via npm would probably make sense to most people contributing. I'm assuming we can easily call lein via npm to build/run/test/etc the server side stuff? 🤔

I wondering if this would be more reliable/easier than bash scripts?

[GentlemanHal]

I'm going to close this PR for now. I think I'll try and make better use of npm scripts as mentioned in my previous comment. This should hopefully fix both of the issue mentioned.

Thanks for submitting it @thesheps ❤️