Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use context interrupts consistently in more places #11511

Merged
merged 4 commits into from
Aug 30, 2024

Conversation

anacrolix
Copy link
Contributor

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 anacrolix marked this pull request as ready for review August 19, 2024 11:01
@anacrolix anacrolix requested review from 0x00101010, zhwrd, mslipper and a team as code owners August 19, 2024 11:01
op-heartbeat/service.go Outdated Show resolved Hide resolved
@anacrolix anacrolix requested a review from protolambda August 20, 2024 06:16
cannon/main.go Outdated Show resolved Hide resolved
@tynes
Copy link
Contributor

tynes commented Aug 23, 2024

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

Copy link
Contributor

@protolambda protolambda left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, few nits, mostly comments that you left and kind of already answered yourself, just need to be addressed to not leave new questions in the code.

op-service/ctxinterrupt/signal-waiter.go Outdated Show resolved Hide resolved
op-service/ctxinterrupt/waiter.go Outdated Show resolved Hide resolved
op-service/ctxinterrupt/waiter.go Show resolved Hide resolved
op-service/ctxinterrupt/funcs.go Outdated Show resolved Hide resolved
op-service/cliapp/lifecycle.go Show resolved Hide resolved
op-service/ctxinterrupt/waiter.go Show resolved Hide resolved
cannon/main.go Outdated Show resolved Hide resolved
cannon/main.go Show resolved Hide resolved
@anacrolix
Copy link
Contributor Author

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 anacrolix enabled auto-merge August 26, 2024 02:49
Copy link
Contributor

@Inphi Inphi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The cannon, op-challenger, and op-dispute-mon changes look good.

op-service/ctxinterrupt/signal-waiter.go Show resolved Hide resolved
cannon/main.go Outdated Show resolved Hide resolved
Copy link
Contributor

@protolambda protolambda left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Added a commit to address the review comments that were left open, and merged in develop to resolve conflicts.

@anacrolix
Copy link
Contributor Author

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

@anacrolix anacrolix added this pull request to the merge queue Aug 30, 2024
@anacrolix
Copy link
Contributor Author

Thanks @zhwrd !

Merged via the queue into develop with commit d520441 Aug 30, 2024
61 checks passed
@anacrolix anacrolix deleted the anacrolix/interrupt-waiting branch August 30, 2024 01:31
samlaf pushed a commit to samlaf/optimism that referenced this pull request Nov 10, 2024
…#11511)

* Use context interrupts consistently in more places

* Fix CI lint errors

(cherry picked from commit 0410b7e)

* op-service/ctxinterrupt: address review comments

---------

Co-authored-by: protolambda <proto@protolambda.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants