Tolerate futures from arbitrary event loops #96
Conversation
|
Can one of the admins verify this patch? |
| @@ -366,15 +366,15 @@ extension HTTPClient { | |||
| /// `EventLoopFuture<Response>` of the execution or cancellation of the execution. | |||
| public final class Task<Response> { | |||
| /// `EventLoop` used to execute and process this request. | |||
| public let eventLoop: EventLoop | |||
| public internal(set) var currentEventLoop: EventLoop | |||
There was a problem hiding this comment.
Now that this is a variable, we must not allow mutation from the public scope.
I thought it might be the right time to set it to currentEventLoop as discussed in the recent comments in #82 now that this property is a variable, but I can also make the name change in another commit if preferred.
cc @weissi @artemredkin
There was a problem hiding this comment.
This looks good but we need some synchronisation here because it is mutable might be accessed from different threads now. I'd suggest to depend on NIOConcurrencyHelpers and store it in
private let _currentEventLoop: AtomicBox<EventLoop>
public var currentEventLoop: EventLoop {
return self._currentEventLoop.load()
}
There was a problem hiding this comment.
Why do we need it to be var? I somehow missed it
There was a problem hiding this comment.
@artemredkin It's because of setChannel(...): when it updates the channel, it must also update the current event loop at the same time.
| @@ -61,23 +61,19 @@ class HTTPClientTests: XCTestCase { | |||
| let response = try httpClient.get(url: "http://localhost:\(httpBin.port)/get").wait() | |||
| XCTAssertEqual(.ok, response.status) | |||
| } | |||
|
|
|||
| func testGetWithSharedEventLoopGroup() throws { | |||
There was a problem hiding this comment.
This shouldn't be needed anymore now that we take care of the threading issues. This test could be re-introduced when we add a requires event loop preference to check that the requirements are always met.
|
@swift-server-bot test this please |
|
@adtrevor there are formatting issues, but otherwise looks good, thank you! |
| @@ -366,15 +366,15 @@ extension HTTPClient { | |||
| /// `EventLoopFuture<Response>` of the execution or cancellation of the execution. | |||
| public final class Task<Response> { | |||
| /// `EventLoop` used to execute and process this request. | |||
| public let eventLoop: EventLoop | |||
| public internal(set) var currentEventLoop: EventLoop | |||
There was a problem hiding this comment.
This looks good but we need some synchronisation here because it is mutable might be accessed from different threads now. I'd suggest to depend on NIOConcurrencyHelpers and store it in
private let _currentEventLoop: AtomicBox<EventLoop>
public var currentEventLoop: EventLoop {
return self._currentEventLoop.load()
}
|
@weissi @artemredkin Thank you for your feedback, I'm going to fix this. |
|
Issues should now be fixed, just waiting to know what you think of the latest changes :). Formatting issues are also probably fixed now. |
|
@swift-server-bot add to whitelist |
|
@swift-server-bot test this please |
The |
|
@artemredkin how does this look to you? @tanner0101 / @ianpartridge / @ldewailly / @tomerd any input on the API change? |
This commit fixes #95 by always hopping event loop futures received from the delegate to the right event loop. This could be a source of bugs if the library users forgot to hop(to:) futures from their delegates implementations.
|
All issues should now be fixed and if no one has additional comments maybe this can now be merged? cc @weissi @artemredkin |
|
@swift-server-bot add to whitelist |
|
@swift-server-bot test this please |
This commit should fix #95 and adds associated tests