Support hopping event loops with EventLoopFuture #177
Merged
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
commented
Mar 16, 2018
Sources/NIO/EventLoopFuture.swift
Outdated
| /// - parameters: | ||
| /// - target: The `EventLoop` that the returned `EventLoopFuture` will run on. | ||
| /// - returns: An `EventLoopFuture` whose callbacks run on `target` instead of the original loop. | ||
| func on(eventLoop target: EventLoop) -> EventLoopFuture<T> { |
There was a problem hiding this comment.
I'm not sure about this name. Maybe hopTo(eventLoop:)?
There was a problem hiding this comment.
Like this PR! What about just hop(eventLoop: EventLoop). So future.hop(eventLoop: otherEL).
There was a problem hiding this comment.
I think I would either call it hopTo(eventLoop:) or just use hopOn(eventLoop:).
There was a problem hiding this comment.
@weissi Any objection to hopTo?
weissi
approved these changes
Mar 16, 2018
Sources/NIO/EventLoopFuture.swift
Outdated
| /// - parameters: | ||
| /// - target: The `EventLoop` that the returned `EventLoopFuture` will run on. | ||
| /// - returns: An `EventLoopFuture` whose callbacks run on `target` instead of the original loop. | ||
| func on(eventLoop target: EventLoop) -> EventLoopFuture<T> { |
There was a problem hiding this comment.
I think I would either call it hopTo(eventLoop:) or just use hopOn(eventLoop:).
Motivation: A somewhat common requirement when working with chains of futures is needing to hop from one event loop to another. This is particularly common when relying on the fact that EventLoopFuture will synchronize with the event loop that created it: you often want to rely on that implicit locking, rather than use Dispatch or Lock to achieve the same effect. While doing this hop requires relatively little code, it's not necessarily totally apparent to new users how they would do it. Additionally, the most naive implementation incurs the overhead of allocations and reference counting in cases where it's not necessary (e.g. when you have only one event loop, or when both work items are being scheduled on the same event loop). For this reason, we should have a nice concise way for a user to request this behaviour and get a relatively performant implementation of the behaviour. Modifications: Added EventLoopFuture<T>.on(eventLoop:). Changed AcceptHandler to use the new method rather than its (slower) alternative. Result: Users will have an easier time working with EventLoopFutures.
Lukasa
force-pushed
the
cb-event-loop-hopping
branch
from
62c746b to
18056d9
Compare
|
Ok, renamed, and also updated |
|
@Lukasa thanks! Shipped :) |
tiagomartinho
pushed a commit
to tiagomartinho/swift-nio
that referenced
this pull request
Mar 17, 2018
Motivation: A somewhat common requirement when working with chains of futures is needing to hop from one event loop to another. This is particularly common when relying on the fact that EventLoopFuture will synchronize with the event loop that created it: you often want to rely on that implicit locking, rather than use Dispatch or Lock to achieve the same effect. While doing this hop requires relatively little code, it's not necessarily totally apparent to new users how they would do it. Additionally, the most naive implementation incurs the overhead of allocations and reference counting in cases where it's not necessary (e.g. when you have only one event loop, or when both work items are being scheduled on the same event loop). For this reason, we should have a nice concise way for a user to request this behaviour and get a relatively performant implementation of the behaviour. Modifications: Added EventLoopFuture<T>.on(eventLoop:). Changed AcceptHandler to use the new method rather than its (slower) alternative. Result: Users will have an easier time working with EventLoopFutures.
weissi
pushed a commit
to weissi/swift-nio
that referenced
this pull request
Feb 3, 2024
Motivation Currently we don't confirm that the decompression has completed successfully. This means that we can incorrectly spin forever attempting to decompress past the end of a message, and that we can fail to notice that a message is truncated. Neither of these is good. Modifications Propagate the message zlib gives us as to whether or not decompression is done, and keep track of it. Add some tests written by @vojtarylko to validate the behaviour. Result Correctly police the bounds of the messages. Resolves apple#175 and apple#176.
# 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.
Motivation:
A somewhat common requirement when working with chains of futures
is needing to hop from one event loop to another. This is particularly
common when relying on the fact that EventLoopFuture will synchronize
with the event loop that created it: you often want to rely on that
implicit locking, rather than use Dispatch or Lock to achieve the
same effect.
While doing this hop requires relatively little code, it's not
necessarily totally apparent to new users how they would do it.
Additionally, the most naive implementation incurs the overhead of
allocations and reference counting in cases where it's not necessary
(e.g. when you have only one event loop, or when both work items are
being scheduled on the same event loop).
For this reason, we should have a nice concise way for a user to
request this behaviour and get a relatively performant implementation
of the behaviour.
Modifications:
Added EventLoopFuture.on(eventLoop:).
Changed AcceptHandler to use the new method rather than its (slower)
alternative.
Result:
Users will have an easier time working with EventLoopFutures.