Rust fun cancellation | Dagger | Page 1

runic kestrel Jun 13, 2024, 3:30 PM

#

Hey Jed. Yes right now we share a reference to the Child process in the query. It should already be behind an arc and mutex if I recall correctly. Currently we just drop it once the arc has no more references. I'd actually assumed that it would kill the process on drop so that is definitely a bug/feature

Yes we can add an explicit disconnect it would require a bit of custom logic but we can in rust even use struct generics to only make methods available on the client if it is active. Think a statemachine.
Yes we can add that kind functionality our best bet currently is to do a blocking drop / cancel to make sure we do what you mention there. And if it hangs the process should either timeout (probably not a good idea as I am guessing clean up might take a bit) or hang until the sdk process is killed itself.

Overall i think ill have a peek and see how the other sdks does their thing. I haven't implemented a custom logging scheme either so right now rust is log by default unlike the goimpl

#

I believe async drop is a nightly only feature so I'll have to work around that if that is the approach we want. Shouldn't be a huge deal tho

minor rivet Jun 13, 2024, 3:36 PM

#

yeah i think the explicit disconnect would be nice, but imo, the big one is that 1. we should make to cleanup the process for sure, and 2. cleanup the process with stdin.close() instead of SIGKILL

#

hm, the session process does get cleaned up actually it looks like

#

as in, it doesn't persist outside the cargo run

#

i wonder if there's some fun process group logic/etc that keeps this working?

#

oh hm i wonder if this works entirely as intended - when the parent exits, the stdin pipe is closed, so 🎉 the session exits neatly

#

but by not closing and waiting explicitly, this can be an issue if the child is pid 1 (or something weird like that)

#

ideally we wouldn't exit the parent until the child exited (like the other sdks)

runic kestrel Jun 13, 2024, 3:56 PM

#

I guess this is as the name implies that the program spawns the process and as such is closed when the pid shuts down for the main program. I've never had issues with it, so I haven't looked into it. But if we want to do some best effort cleanup in the engine then I think we should do it more explicitly

minor rivet Jun 13, 2024, 4:00 PM

#

Agreed 🎉 is there any chance you could take a look? I'm also happy to try and stumble my way through it, but my rust knowledge is definitely not very great

runic kestrel Jun 13, 2024, 4:18 PM

#

I'll have a look. I'll shoot you a review / update whatever i end up finding out

steel haven Jun 13, 2024, 5:21 PM

#

minor rivet but by not closing and waiting explicitly, this *can* be an issue if the child i...

yeah I think this is what's happening - the process just gets killed when the container exits

steel haven Jun 13, 2024, 5:23 PM

#

runic kestrel I guess this is as the name implies that the program spawns the process and as s...

normally it doesn't matter much - this mostly affects our test suite tbh, since we run Dagger in Dagger (dagger CLI against a dagger engine running as a service in Dagger). But yeah, closing stdin and waiting for it to exit is the best practice to ensure all telemetry is flushed by the time the client exits, which can sometimes be important even aside from our particular test setup

runic kestrel Jun 24, 2024, 6:11 PM

#

Here you go

https://github.com/dagger/dagger/pull/7736/files

GitHub

feat: add explicit stop to dagger process by kjuulh · Pull Request ...

Based on a request that if a rust binary is spawned as PID 1 without closing stdin on the dagger session it doesn't cleanup properly after itself

steel haven Jun 24, 2024, 6:15 PM

#

runic kestrel Here you go https://github.com/dagger/dagger/pull/7736/files

awesome thanks! I might have missed it, is there a close(stdin) in there somewhere?

runic kestrel Jun 24, 2024, 6:15 PM

#

The wait automatically closes stdin of the process and waits for it to complete 😄

steel haven Jun 24, 2024, 6:16 PM

#

ha neat

#

indeed it does: https://docs.rs/tokio/latest/tokio/process/struct.Child.html#method.wait

Child in tokio::process - Rust

Representation of a child process spawned onto an event loop.

runic kestrel Jun 24, 2024, 6:16 PM

#

Waits for the child to exit completely, returning the status that it exited with. This function will continue to have the same return value after it has been called at least once.

The stdin handle to the child process, if any, will be closed before waiting. This helps avoid deadlock: it ensures that the child does not block waiting for input from the parent, while the parent waits for the child to exit.

If the caller wishes to explicitly control when the child’s stdin handle is closed, they may .take() it before calling .wait():

#

Pretty cool I think 😄

steel haven Jun 24, 2024, 6:17 PM

#

we just naturally chose such an ergonomic interface! 😛

runic kestrel Jun 24, 2024, 6:17 PM

#

I've tested it and it does seem to cleanup the session before returning

#

All good. I needed a bit of a hack as it isn't possible to block a task in a drop. But hopefully that will be fine ones async drop is stable. It just went into nightly last week or so, so it will probably be a while

And didn't want to either change the ui, or implement a complicated solution

#

Interesting first time running rust generate from dagger call. xD

What could go wrong

runic kestrel Jun 24, 2024, 7:52 PM

#

Okay I didn't manage to fully succeed. Somehow it seems that the process is being kept alive even tho it closes the stdin

Nvm figured it out. The hack I did doesn't work on a single threaded runtime

#

I'll continue tomorrow

#Rust fun cancellation