Better align shutdown semantics of testing event loops #2800
+224
−108
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Lukasa
added
the
patch-version-bump-only
Lukasa
reviewed
Jul 19, 2024
| @@ -95,6 +95,10 @@ public final class NIOAsyncTestingEventLoop: EventLoop, @unchecked Sendable { | |||
| /// The queue on which we run all our operations. | |||
| private let queue = DispatchQueue(label: "io.swiftnio.AsyncEmbeddedEventLoop") | |||
|
|
|||
| private enum State: Int, AtomicValue { case open, closing, closed } | |||
| private let _state = ManagedAtomic(State.open) | |||
| private var state: State { self._state.load(ordering: .relaxed) } | |||
There was a problem hiding this comment.
The atomic ordering here seems likely to be logically wrong. We should probably force ourselves to express it at all points.
| @@ -150,6 +154,15 @@ public final class NIOAsyncTestingEventLoop: EventLoop, @unchecked Sendable { | |||
| let promise: EventLoopPromise<T> = self.makePromise() | |||
| let taskID = self.scheduledTaskCounter.loadThenWrappingIncrement(ordering: .relaxed) | |||
|
|
|||
| switch self.state { | |||
There was a problem hiding this comment.
The load here should be acquiring.
| @@ -324,9 +334,11 @@ public final class NIOAsyncTestingEventLoop: EventLoop, @unchecked Sendable { | |||
|
|
|||
| /// The concurrency-aware equivalent of `shutdownGracefully(queue:_:)`. | |||
| public func shutdownGracefully() async { | |||
| await withCheckedContinuation { (continuation: CheckedContinuation<Void, Never>) in | |||
| await withCheckedContinuation { continuation in | |||
| self._state.store(.closing, ordering: .relaxed) | |||
There was a problem hiding this comment.
These stores should be releasing.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Motivation:
The two testing event loops—
EmbeddedEventLoopandNIOAsyncTestingEventLoop—have different semantics for outstanding work during shutdown, which are both different from the productionSelectableEventLoop, specifically with newly scheduled tasks that result from running existing scheduled work at the time of shutdown.There are three axes to consider:
SelectableEventLoopEmbeddedEventLoopNIOAsyncTestingEventLoopNote that
NIOAsyncTestingEventLoopmay never terminate because of this.Modifications:
This PR aligns
EmbeddedEventLoopandNIOTestingEventLoopand makes them more similar toSelectableEventLoopand less surprising semantics.Result:
SelectableEventLoopEmbeddedEventLoopNIOAsyncTestingEventLoopFuture work:
Given both the
EmbeddedEventLoopandNIOAsyncTestingEventLoopare used in tests of NIO applications that run withSelectableEventLoopin production, it might be worth extending both to also support quiescing to give a more representative shutdown behaviour in tests.