Add inverse of preconditionInEventLoop() and ability to add messages to the assertion
#1508
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
Motivation: It is valuable to be able to provide additional context for any kind of assertion failure, such as obvious points of API misuse or when the purpose of the assertion itself is unclear. Modifications: Added an overload of preconditionInEventLoop() that accepts a message autoclosure parameter in the same fashion as `Swift.precondition(_:file:line:)`. Default the implementation of the original version to calling the new one with an empty message. Add defaulted implementation of the new version which forwards the message closure to precondition(). Add the new message parameter directly to assertInEventLoop() without a forwarder since it is not a customization point. Result: It is now possible to optionally provide explanatory messages to be included when the "in event loop" precondition fails, as with any other precondition.
…or clarity. Motivation: The existing comments were grammatically a bit awkward. Modifications: Changed the comments. Result: The documentation comments are more grammatically correct and technically precise.
…o EventLoop. Motivation: As it is useful to have a precondition to assert that calling code *is* on the given EventLoop, it is also useful to assert that calling code is *not* on it. Modifications: Add two new methods to EventLoop, one of them as a protocol method, both with default implementations. Result: It is now possible to canonically assert that a given execution path is *not* executing on a given event loop (including providing an explanatory message, as with the earlier changes to the counterpart methods). This provides better parity with dispatchPrecondition() and enables replacing direct access to `EventLoop.inEventLoop` with a direct statement of intent providing guaranteed semantics.
…propriate locations. Motivation: This is the purpose for which the new methods were intended: To replace handwritten conditions with direct statements of assertion intent. Modifications: Replaced various uses of precondition(!eventLoop.inEventLoop) and similar assert()s with the respective *notInEventLoop methods. In the case of the usage in EventLoopFuture.wait(), ensure the explainer message is provided on precondition failure. Result: The modified code is clearer, explicitly declares intent, can benefit from customizations provided by individual event loop types (such as the use of dispatchPrecondition() in NIOTSEventLoop), and can be more easily refactored.
|
Filed #1509 to track crash-detecting integration tests that cover all of NIO's codebase. |
|
The failure of the API breakage test is expected given the new APIs added and the source-compatible change of |
Lukasa
requested changes
May 6, 2020
A PR addressing the implementation for NIOTS is in progress. |
|
First draft of changes for NIOTS is up. Needs significant additional work. |
Lukasa
approved these changes
May 7, 2020
Lukasa
pushed a commit
to apple/swift-nio-transport-services
that referenced
this pull request
May 7, 2020
Motivation: See apple/swift-nio#1508 for rationale Modifications: Provides implementations of `preconditionInEventLoop(_:file:line:)` and `preconditionNotInEventLoop(_:file:line:)`. Result: NIOTSEventLoop will now use dispatchPrecondition(condition:) for the in/not in preconditions instead of relying on the default implementation, yielding improved correctness.
weissi
requested changes
May 7, 2020
weissi
reviewed
May 7, 2020
…both existing and new precondition methods on EventLoop, including pulling the new overloads entirely. Add some missing @inlinable attributes while at it. Revert EventLoopFuture.wait() to using precondition() directly to ensure its explainer message is shown.
Lukasa
pushed a commit
to apple/swift-nio-transport-services
that referenced
this pull request
May 7, 2020
… removing the message parameter from the precondition assertion methods entirely. Added a couple of missing @inlinables to match the declarations now present over there as well. (#85)
|
Cool, I think we think the API breakage checker is wrong for known reasons, so I'm going to merge this. |
pull bot
pushed a commit
to scope-demo/swift-nio
that referenced
this pull request
May 11, 2020
…s to the assertion (apple#1508) Motivation: It is valuable to be able to provide additional context for any kind of assertion failure, such as obvious points of API misuse or when the purpose of the assertion itself is unclear. Modifications: Added an overload of preconditionInEventLoop() that accepts a message autoclosure parameter in the same fashion as `Swift.precondition(_:file:line:)`. Default the implementation of the original version to calling the new one with an empty message. Add defaulted implementation of the new version which forwards the message closure to precondition(). Add the new message parameter directly to assertInEventLoop() without a forwarder since it is not a customization point. Result: It is now possible to optionally provide explanatory messages to be included when the "in event loop" precondition fails, as with any other precondition.
# for free
to join this conversation on GitHub.
Already have an account?
# 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.
Add
preconditionNotInEventLoop(_:file:line:)andassertNotInEventLoop(_:file:line:), and add optional message parameter to the existing counterparts.WARNING: There are no tests for the existing or new precondition APIs, as unit tests can not test crash conditions and the integration tests currently can not build all of NIO when testing crashes. An issue will be filed to track addressing this shortcoming.
Motivation:
It is valuable to be able to provide additional context for any kind of assertion failure, such as obvious points of API misuse or when the purpose of the assertion itself is unclear. The explainer message provided on precondition failure in
EventLoopFuture.wait()is an ideal example.As it is useful to have a precondition to assert that calling code is on the given
EventLoop, it is also useful to assert that calling code is not on thatEventLoop. Precedent is provided by thedispatchPrecondition()API.Modifications:
preconditionInEventLoop()that accepts amessage@autoclosureparameter in the same fashion as doesSwift.precondition(_:file:line:). The overload's implementation calls that very function with the appropriate inputs, including the forwarded message (if any). The implementation of the original version now calls through to the new one with an empty message, providing 100% source compatibility.assertInEventLoop(). Didn't bother with a forwarder, as the method is not specified on theEventLoopprotocol; just gave the new param a default value.EventLoop, providing the precondition form as a protocol requirement and the assertion form only as an extension method, to match their counterparts.messageautoclosure inputs but, being newly added, default the argument rather than adding forwarders.precondition(!eventLoop.inEventLoop)andassert(!eventLoop.inEventLoop)byEventLoopFutureandNIOHTTP1TestServerwitheventLoop.preconditionNotInEventLoop()andeventLoop.assertNotInEventLoop()respectively.Result:
Swift.precondition()API.dispatchPrecondition(), allows replacing direct access toEventLoop.inEventLoopwith direct statements of intent, and enables overriding the assertion behaviors on a per-loop basis (such as the use ofdispatchPrecondition()byNIOTSEventLoop, not shown in this PR of course),