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

op-challenger: cleanup service lifecycle, improve subscription ctx usage #8235

Merged
merged 1 commit into from
Nov 22, 2023

Conversation

protolambda
Copy link
Contributor

Description

Clean up the challenger service lifecycle:

  • register interrupt signal handling immediately at main, and attach it as value to ctx, for the app lifecycle to utilize
  • turn game.Service into a service with a lifecycle; explicit Start / Stop methods.
  • Make the game.Service close all its initialized resources.
  • Use the same pattern as batcher/proposer/op-node/indexer: init from config, and if there's an error during initialization, try to close what's left open of the service.
  • Now that the challenger has a lifecycle, the test Helper doesn't need to maintain a channel and context, and can just stop the challenger.

Improve related op-service utils:

  • improve WatchHeadChanges: unsubscribe signals are now handled in parallel to the subscription tasks, and cancel the context internally. This way an "unsubscribe" really is an unsubscribe, and we don't also have to keep track of a long-running context that has to be closed prior to unsubscribing. The ctx attribute is only used for the duration of the call that creates the subscription, and does not affect the subscription after that.
  • similarly, fix PollBlockChanges to also handle unsubscribe signals like that
  • add a Close() method to the simple tx-manager, so services can shut down the underlying RPC client connection.

With the above op-service fixes:

  • Also close the subscriptions in op-node properly, don't use the resource ctx for
  • Close the tx-manager in op-proposer and op-batcher

Tests

No functionality changes, just cleanup. Open for suggestions to add tests where previously missing.

Metadata

Part of #7671

@protolambda protolambda added the A-op-challenger Area: op-challenger label Nov 22, 2023
@protolambda protolambda requested a review from ajsutton November 22, 2023 00:31
@protolambda protolambda requested a review from a team as a code owner November 22, 2023 00:31
Copy link
Contributor

@ajsutton ajsutton left a comment

Choose a reason for hiding this comment

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

LGTM. Really nice clean up.

Copy link

codecov bot commented Nov 22, 2023

Codecov Report

Merging #8235 (1e157a7) into develop (2e35a8f) will increase coverage by 7.81%.
Report is 24 commits behind head on develop.
The diff coverage is n/a.

Additional details and impacted files
@@             Coverage Diff             @@
##           develop    #8235      +/-   ##
===========================================
+ Coverage    53.44%   61.26%   +7.81%     
===========================================
  Files          162       86      -76     
  Lines         6040     1931    -4109     
  Branches       964      438     -526     
===========================================
- Hits          3228     1183    -2045     
+ Misses        2690      672    -2018     
+ Partials       122       76      -46     
Flag Coverage Δ
cannon-go-tests ?
chain-mon-tests ?
common-ts-tests ?
contracts-bedrock-tests 61.26% <ø> (ø)
contracts-ts-tests ?
core-utils-tests ?
sdk-next-tests ?
sdk-tests ?

Flags with carried forward coverage won't be shown. Click here to find out more.

see 76 files with indirect coverage changes

@ajsutton ajsutton added this pull request to the merge queue Nov 22, 2023
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to no response for status checks Nov 22, 2023
@ajsutton
Copy link
Contributor

I suspect the flaky test from the merge queue failure will be fixed by #8217

@ajsutton ajsutton added this pull request to the merge queue Nov 22, 2023
Merged via the queue into develop with commit bce7fb5 Nov 22, 2023
60 checks passed
@ajsutton ajsutton deleted the challenger-lifecycle branch November 22, 2023 02:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-op-challenger Area: op-challenger
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants