optimism icon indicating copy to clipboard operation
optimism copied to clipboard

Use context interrupts consistently in more places

Open anacrolix opened this issue 1 year ago • 2 comments

This was a fix that came out of fixing the shutdown handling when testing various services.

Mainly I made the signal use for interrupting contexts consistent, it was using different signals in different places, and not always using the interrupt waiter interface even though that was the intent.

There were a few places were unnecessary goroutines were spun out and cancellation was missed or not handled as intended. There's also a place where the context wasn't being used and a new cancellation method introduced to work around it, which wasn't the intent I think it was just missed in a refactor.

The rename from opio to ctxinterrupt seems a bit noisy but I think it conveys the intent better and removes some stutter around the naming.

anacrolix avatar Aug 19 '24 08:08 anacrolix

This PR looks good to me altho I do not own a bunch of this code. The cannon diff is important to have somebody look at

tynes avatar Aug 23 '24 01:08 tynes

I think the only pending question is whether the stdout punting is still wanted (someone might be expecting to see the "exiting..." message while using cannon. If so it would make sense to do that everytime an interrupt is received, probably by providing a small callback in cannon so that you only see the message on stdout if the signal is actually being handled by whoever is waiting on the interrupts.

anacrolix avatar Aug 26 '24 02:08 anacrolix

Looks good. This is still stuck on a code owner 😬 @mslipper @zhwrd @0x00101010 ?

anacrolix avatar Aug 30 '24 01:08 anacrolix

Thanks @zhwrd !

anacrolix avatar Aug 30 '24 01:08 anacrolix