Run the Kitura router on a DispatchQueue instead of an ELT #78
Conversation
| @@ -23,7 +23,7 @@ import Foundation | |||
| public class HTTPServerResponse: ServerResponse { | |||
|
|
|||
| /// The channel handler context on which an HTTP response should be written | |||
| private weak var ctx: ChannelHandlerContext? | |||
| private weak var channel: Channel? | |||
There was a problem hiding this comment.
The comment above needs editing.
| if let delegate = weakSelf.server.delegate { | ||
| Monitor.delegate?.started(request: weakSelf.serverRequest, response: weakSelf.serverResponse) | ||
| delegate.handle(request: weakSelf.serverRequest, response: weakSelf.serverResponse) | ||
| } //TODO: failure path |
There was a problem hiding this comment.
Can we sort out this failure TODO now?
There was a problem hiding this comment.
Yeah, I think we can use the HTTPDummyServerDelegate here. We use that if no delegate is registered.
| delegate.handle(request: serverRequest, response: serverResponse) | ||
| } //TODO: failure path | ||
| DispatchQueue.global().async { [weak self] in | ||
| guard let weakSelf = self else { fatalError("No HTTPHandler to proceed with the response.") } |
There was a problem hiding this comment.
Is fatalError() essential or could we carry on?
There was a problem hiding this comment.
Right, we don't need a fatalError there. I have replaced it with a Log.error. I think there's a lot of other places that need this change. Will do a cleanup PR soon.
| } | ||
|
|
||
| } | ||
| //func testBadRequestFollowingGoodRequest() { |
There was a problem hiding this comment.
We don't need to comment out the code of the test, just the entry in the test array.
There was a problem hiding this comment.
I think that doesn't work on macOS. I think Xcode doesn't pick the tests from the array, it runs all tests found in a file. I may be wrong - just trying a travis CI run.
There was a problem hiding this comment.
It failed: https://travis-ci.org/IBM-Swift/Kitura-NIO/jobs/419978530
Reverting back.
There was a problem hiding this comment.
I'm sorry, you were right!
pushkarnk
force-pushed
the
handler-on-dispatch
branch
2 times, most recently
from
71ed702 to
8b4b8f9
Compare
|
@ianpartridge Thanks for the review! I've addressed all of your comments. |
pushkarnk
force-pushed
the
handler-on-dispatch
branch
2 times, most recently
from
4569fcb to
5dc374a
Compare
Currently, the Kitura router that eventually invokes the endpoint handlers runs on the event loop threads. This is bad design primarily because the handlers may invoke blocking code which will end up blocking the event loop threads, reducing their availability to handle newer HTTP requests.
This pull request proposes moving the Kitura router invocation to a DispatchQueue, effectively freeing up the event loop thread which was scheduled to handle the related HTTP request. We move back to the event loop while writing the response -
HTTPServerResponse.end(). Error handling (error responses) is done on the event loop. To write responses we useChannels, instead ofChannelHandlerContexts, owing to the thread-safety provided by them.