-
Notifications
You must be signed in to change notification settings - Fork 29k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
quic: add quic #38233
quic: add quic #38233
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
EndpointWrap
is the JS Wrapper object aroundEndpoint
. It is designed to be cloneable viaMessagePort
. What that means is that anEndpoint
(and it's associateduv_udp_t
can essentially be shared across multiple worker threads while still being owned by a single event loop). It makes the implementation a bit more complicated but opens up the ability for processing QUIC sessions across multiple worker threads.
This doesn't really seem future-proof in a world in which we will eventually be able to transfer handles between threads. Using another thread's I/O facilities kind of breaks the mental model that people have of Workers and renders them fairly pointless in general.
Stream
is no longer aStreamBase
. In fact, while the QUIC implementation will be capable of sending from aReadable
stream, and will support being read as aReadable
, the QUIC JavaScriptStream
object will not be a Node.js stream object. This is a major API change from the original implementation. More details on this later, but the usage pattern will be closer to what we see in browser environments (e.g.stream.blob()
,stream.readable()
,stream.text()
,
Gonna be honest, this feels like something that's going to come back to bite us, both from an API perspective (special-casing QUIC vs making the API as close as possible to the TCP one is going to make everything harder for users, not easiers) and from an internals perspective (having some consistent stream API on the C++ side does make sense!).
static std::shared_ptr<SocketAddress> FromSockName(const uv_udp_t& handle); | ||
static std::shared_ptr<SocketAddress> FromSockName(const uv_tcp_t& handle); | ||
static std::shared_ptr<SocketAddress> FromPeerName(const uv_udp_t& handle); | ||
static std::shared_ptr<SocketAddress> FromPeerName(const uv_tcp_t& handle); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you explain why? SocketAddress
is copyable, and copying it would be expected to have lower performance overhead than a separate allocation
(And even if not ... why std::shared_ptr
instead of std::unique_ptr
?)
Will we? It's been discussed but I hadn't seen any activity to that end so I didn't really want to bank on that arriving at any time in the near future. If/when that time does come, it can be accommodated under this model by modifying
Why? Can you explain? It's not far off from the model we use in Piscina, for instance, where we keep the i/o on the main thread and dispatch the compute to workers.
Having been through all this code many times, I disagree entirely. And I don't plan on special casing the QUIC code -- I plan on revisiting both the HTTP/2 and HTTP/1 APIs to align them closer with this model. In other words, I'm intentionally moving away from the existing streams API (in both internal and external APIs). |
I mean, "nobody's working on it so it's not going to happen" is ... Yes, it's unlikely that we're going to get a nice API for this on the libuv side anytime soon, something that I had been waiting/hoping for, but we can always choose to do the same things we do for child processes. That's not trivial either, but it's still way less complex than adding special casing for each class that wraps a libuv handle.
Because in that case the thread whose event loop owns the handle also performs any active I/O calls -- it's the exact opposite of what piscina does. As an extreme example, when the handle-owning becomes blocked, other threads won't be able to actually make use of the handle. This is a blocker, imo.
You can feel free to disagree, but please don't add APIs without having a plan for how to integrate them with the existing streams API that has consensus among collaborators beforehand. |
That's why this is all being done in the open with visibility through a draft PR. |
Well... in that case, what's the state that you eventually want to reach in terms of internal streams API? I mostly really want to avoid a scenario in which one chunk of the code base uses one streams model and another uses another streams model. It's bad enough that we have streams like zlib or fs streams that don't align with other code, but let's not make it worse. |
Let's start with a look at the external API then work our way in. Let's look at server-side once a session has been created and we've received a stream from the peer... stream.respondWith({ body: fs.promises.open('file') }); // Promise<FileHandle>
stream.respondWith({ body: await fs.promises.open('file') }); // FileHandle
stream.respondWith({ body: fs.openReadStream('...') }); // any stream.Readable
stream.respondWith({ body: blob }); // buffer.Blob
stream.respondWith({ body: buffer }); // Buffer/TypedArray/ArrayBuffer
stream.respondWith({ body: 'hello world' }); // string
stream.blob() // get the stream contents as a Blob
stream.readable() // get the stream contents as a stream.Readable
stream.text() // get the stream contents as a string
// ... On the open stream side: session.openStream({ unidirectional: true, body: fs.promises.open('file') }); // Promise<FileHandle>
session.openStream({ unidirectional: true, body: fs.openReadStream('file') }); // stream.Readable
session.openStream({ unidirectional: true, body: 'hello world' }); // string
// ... This is largely mockup currently because I'm still working through the details and ergonomics but it gives a basic idea. Rather than the QUIC From here, on the internals, the implementation will be capable of consuming from any As you are already aware, internally, the QUIC implementation already has to be different because of the way ngtcp2 works. Outbound data has to be queued internally with potentially multiple passes at attempting to read. Whenever a QUIC packet is serialized, we pass ngtcp2 a pointer to the available data and it decides how much it wishes to consume, we are then required to retain the sent data in memory in case ngtcp2 determines that there has been packet loss. Any single chunk of outbound data could be passed multiple times into ngtcp2, and any packet serialization could be smaller or larger than any single Further, there is a bottleneck currently built into the JS to StreamBase design in that it will queue outbound writes at the JS side until the previous write callback is invoked. However, once invoked the Buffer reference is released, allowing it to be garbage collected. That means we cannot invoke the callback until after the data is acknowledged, which bottlenecks our outbound data flow. StreamBase currently does not give us access to the Buffer to force it to be retained -- it only gives us a set of uv_buf_t's. This means that, when using Stream, the outbound data ends up needlessly being buffered twice and causes our quic stream to be starved of data until all outstanding acks come in. So while the api will support consuming the existing JS to StreamBase model, it won't be limited by it. When reading from a native StreamBase instance, such as a FileHandle, we thankfully won't have that limitation. Reading from those will be straightforward, pretty simple, and very performant -- tho it still needs a bridge into the pull model required by ngtcp2. |
45c72aa
to
464bef2
Compare
0de87f7
to
3c62871
Compare
980fc98
to
df7489a
Compare
Signed-off-by: James M Snell <jasnell@gmail.com>
@jasnell may I make a suggestion? That before the code base becomes too different you take the time to review the fixes we implemented. I know theres a bit there, however they are required to get the previous code base to run reasonably well (we did both demo's with it and now have a prototype fleet running it in development). |
Hey @splitice ! Yeah I actually have all of your fixes open in a set of tabs that I'm going to start going through later this week :-) |
@jasnell then I appologise in advance for all my sins :) |
Heh, don't apologize! Based on all the work you've done I owe you the beverage of your choice if we ever end up in the same location face to face. |
@jasnell I havent merged your squashed build changes however 2 more for the list.
The following test script is supplied:
Feedback / gut feelings welcome @jasnell . I can spend some time investigating tomorrow and any direction you can supply would be very helpful. |
A setTimeout to delay process exit after So a perhaps the destruction of the handle due to process exit while the handle is being closed. Maybe no reference being maintained for the close cb? That would make sense I think. Also that close cb should resolve the closed promise (which is resolved earlier!). |
Fix for issue 13 Glad I'm not paid by line of code for that one. I'm embarrased to admit how long it took to find. |
Unfortunately, I think I need to accept the fact that I'm not likely to get back to this PR any time in the near future (or likely this year). Try as I might, this is just going to take way too much time to finish up... Therefore, I'd like to ask if there is anyone willing to take this activity over and get the QUIC implementation to completion. |
Hi @jasnell, I am not a C/C++ developer, but I am would be open to contribute with anything non-C/C++ related. |
I know it's hard to do but a roadmap to (potential) merge would be good. Particularly if the new project leader for this contribution was not a regular Node contributor. Unfortunately it's not likely to be me. The client I consulted with for the work to date does not have a sufficient budget for it. However I'm happy to continue to report and/or fix any bugs discovered in our prototype client. Currently despite the few minor known issues remaining it actually works pretty well. Our prototype has got quite the use internally. |
In the next couple of weeks I will put something together. I've been talking to @mcollina this week about this and there likely is at least some part that I can continue pushing forward but help will be needed to finish it. So either way I'll need to put together a roadmap. Give me a week or two to get that together |
Ok, so update. I am going to split this PR into two. The first will contain only the c++ pieces, the other will contain the JavaScript apis, tests, and JavaScript docs. I will keep iterating on the c++ pieces myself starting next week, but will have to leave the JavaScript pieces to someone else to help. By the end of next week I will have the roadmap documented on the remaining todos so folks can help. Initially, to make reviewing and landing this early, I suggest that we not worry about making sure that every bug is squashed before this lands. We can iterate after to make sure everything gets done as we go. As before, I'm happy to jump on to calls describing what is here but I will also work on a detailed documentation of the design also. |
Hi @jasnell, I like the strategy you’ve outlined, it will help split the work into smaller chunks. 🙂🙂🙂 |
@jasnell looking forward to the plan! I was wondering whether just using the msquic (https://github.com/microsoft/msquic) library would speed up development? Or is it because of inconsistencies in the C/C++ codebase in nodejs if using a big external library. |
@CMCDragonkai why would it be a better fit than ngtcp2 (which is already in use)? |
Just FYI... I've started working on this again. It'll be slow going but I am making progress. |
This PR is superseded by #44325 |
Working on adding QUIC back. This is a bit of a rewrite of the original and is not yet complete (thus the draft status).
There are several important bits here:
The internal classes are
Endpoint
,EndpointWrap
,Session
, andStream
(originally these were namedQuicSocket
,QuicSession
, andQuicStream
.Endpoint
andEndpointWrap
are a bit special...Endpoint
wraps auv_udp_t
but does not wrapUDPWrap
. The reason is to give us finer control over how the UDP port is used and configured, and how data flows.EndpointWrap
is the JS Wrapper object aroundEndpoint
. It is designed to be cloneable viaMessagePort
. What that means is that anEndpoint
(and it's associateduv_udp_t
can essentially be shared across multiple worker threads while still being owned by a single event loop). It makes the implementation a bit more complicated but opens up the ability for processing QUIC sessions across multiple worker threads.Stream
is no longer aStreamBase
. In fact, while the QUIC implementation will be capable of sending from aReadable
stream, and will support being read as aReadable
, the QUIC JavaScriptStream
object will not be a Node.js stream object. This is a major API change from the original implementation. More details on this later, but the usage pattern will be closer to what we see in browser environments (e.g.stream.blob()
,stream.readable()
,stream.text()
, etc).There's
noonly partial JS API exposed hereyet. That's coming as I work through the rewrite.