reorganize handle_child

[sqwishy]

There were a couple issues with the current implementation:

1. When reading stdout and stderr from the child, as soon as we hit EOF on one we would stop reading from both (line 1420). This could lead to the return value not being read from the job program.

2. Lines read from stdout and stderr are put into a channel and read elsewhere with rx.recv() (line 1497) but that channel isn't read until empty. It is only read in the while !done.load(...) (line 1449) loop and that loop can stop after any .store(true, ...). Which happens when the child exits, when the job is cancelled, when either stdout or stderr reach EOF...

   This can be verified by putting dbg!(rx.recv().await) or a similar assertion after the while loop before returning from that function. It shows the channel still containing log lines on rare occasions.

I was pretty careful in this to maintain the current behaviour; adding comments to express intention.

One difference in this is that some regular intervals (cancel check and ping update) should be more regular?

Before...

at 00ms wait for 10ms at 10ms do things for 3ms at 13ms wait again for 10ms at 23ms do things again ...

With change...

at 00ms wait for 10ms at 10ms do things for 3ms at 13ms wait again but for 7ms at 20ms do things again ...

Which I'm guessing is preferable but I could be wrong.

[rubenfiszel]

at 00ms wait for 10ms
at 10ms do things for 3ms
at 13ms wait again but for 7ms
at 20ms do things again ...

This seems preferable indeed but need to look back at the code why it is the case

[sqwishy]

This fucking piece of shit CI dude.

Also @rubenfiszel this is an example of some nsjail jank that I've seen locally sometimes.

2022-09-20T23:08:36.8978994Z   left: `Object {"error": String("Error during execution of the script\nlast 5 logs lines:\n[I][2022-09-20T23:08:36+0000] Executing '/usr/local/bin/python3' for '[STANDALONE MODE]'\n[E][2022-09-20T23:08:36+0000][1] void subproc::subprocNewProc(nsjconf_t*, int, int, int, int, int)():211 execve('/usr/local/bin/python3') failed: No such file or directory\n[F][2022-09-20T23:08:36+0000][1] pid_t subproc::runChild(nsjconf_t*, int, int, int, int)():466 Launching child process failed\n[W][2022-09-20T23:08:36+0000][11929] pid_t subproc::runChild(nsjconf_t*, int, int, int, int)():486 Received error message from the child process before it has been executed\n[E][2022-09-20T23:08:36+0000][11929] int nsjail::standaloneMode(nsjconf_t*)():272 Couldn't launch the child process")}`,

aka

[I][2022-09-20T23:08:36+0000] Executing '/usr/local/bin/python3' for '[STANDALONE MODE]'
[E][2022-09-20T23:08:36+0000][1] void subproc::subprocNewProc(nsjconf_t*, int, int, int, int, int)():211 execve('/usr/local/bin/python3') failed: No such file or directory
[F][2022-09-20T23:08:36+0000][1] pid_t subproc::runChild(nsjconf_t*, int, int, int, int)():466 Launching child process failed
[W][2022-09-20T23:08:36+0000][11929] pid_t subproc::runChild(nsjconf_t*, int, int, int, int)():486 Received error message from the child process before it has been executed
[E][2022-09-20T23:08:36+0000][11929] int nsjail::standaloneMode(nsjconf_t*)():272 Couldn't launch the child process

[rubenfiszel]

@sqwishy I think for this error we might want to reproduce this under nsjail more verbose/debug mode. It's pretty annoying, I hope we will not have to patch nsjail...

[sqwishy]

@rubenfiszel Any changes regarding nsjail debugging will go in a separate branch. CI appears to be failing because it's using an old Go compiler.

If you're fine with everything else in the changes here you can merge it or if you want the CI to pass I can look at writing our Go bootstrapping code to support older compilers and put that in another pull request, let me know.

[rubenfiszel]

I'm in the plane, i will fix CI when i land :)

On Wed, Sep 21, 2022, 18:47 sqwishy @.***> wrote:

@rubenfiszel https://github.com/rubenfiszel Any changes regarding nsjail debugging will go in a separate branch. CI appears to be failing because it's using an old Go compiler.

If you're fine with everything else in the changes here you can merge it or if you want the CI to pass I can look at writing our Go bootstrapping code to support older compilers and put that in another pull request, let me know.

— Reply to this email directly, view it on GitHub https://github.com/windmill-labs/windmill/pull/606#issuecomment-1254032018, or unsubscribe https://github.com/notifications/unsubscribe-auth/AACDJAG2AF2EFMNWI3BRVNTV7NC37ANCNFSM6AAAAAAQQVTUNE . You are receiving this because you were mentioned.Message ID: @.***>