Prototype pipe handling without dispatch IO #2315
Conversation
|
|
||
| private let receiveIO: DispatchIO | ||
| private let inFD: FileHandle | ||
| private let sendIO: DispatchIO |
There was a problem hiding this comment.
I think that we should replace this to be symmetric with the read side.
There was a problem hiding this comment.
Thanks a lot for this PR. Apart from this fixing the Windows thread spinning issue (which is amazing), this looks like a great simplification to me 🎉. I have left a few comments inline.
If we take this, I think we should also switch sendIO to be backed by FileHandle instead of DispatchIO but that can be a follow-up PR to keep this one focused.
|
|
||
| guard let data = data, !data.isEmpty else { | ||
| self.inFD.readabilityHandler = { fileHandle in | ||
| let data = (try? fileHandle.read(upToCount: Int.max)) ?? Data() |
There was a problem hiding this comment.
Instead of try?, I would prefer to use orLog so that we log the error thrown by read(upToCount:).
| } | ||
| } | ||
|
|
||
| self.queue.sync { |
There was a problem hiding this comment.
self.queue.async should be sufficient here, it still guarantees that all incoming data is handled in-order and frees up the thread that the readability handler is running on. Or do you think I’m missing something?
| /// Buffer of received bytes that haven't been parsed. | ||
| /// | ||
| /// Access to this must be be guaranteed to be sequential to avoid data races. Currently, all access are | ||
| /// - The `receiveIO` handler: This is synchronized on `queue`. |
There was a problem hiding this comment.
We now have a corresponding use in self.inFD.readabilityHandler, so we should include that here:
/// - The `inFD.readabilityHandler`: This is synchronized on `queue`.|
|
||
| logger.log("Closing JSONRPCConnection...") | ||
| // Attempt to close the reader immediately; we do not need to accept remaining inputs. | ||
| receiveIO.close(flags: .stop) |
There was a problem hiding this comment.
Should we set the readabilityHandler of inFD to nil here to ensure that we don’t receive any more messages? If we do so, I think we also no longer need to set fileHandle.readabilityHandler = nil if data.isEmpty in the readabilityHandler.
| private let sendQueue: DispatchQueue = DispatchQueue(label: "jsonrpc-send-queue", qos: .userInitiated) | ||
|
|
||
| private let receiveIO: DispatchIO | ||
| private let inFD: FileHandle |
There was a problem hiding this comment.
Should we match the previous naming and call this receiveFD? inFD can get confusing if you eg. set up a JSONRPCConnection from SourceKit-LSP to a BSP server, in which case the receiveFD will be stdout of the BSP server process and thus receiveFD wouldn’t be stdin.
| let ioGroup = DispatchGroup() | ||
|
|
||
| #if os(Windows) | ||
| let rawInFD = dispatch_fd_t(bitPattern: inFD._handle) |
3 participants
No description provided.