Skip to content
This repository has been archived by the owner on Aug 11, 2020. It is now read-only.

Iteration 6 #31

Closed
wants to merge 25 commits into from
Closed

Iteration 6 #31

wants to merge 25 commits into from

Conversation

jasnell
Copy link
Member

@jasnell jasnell commented May 20, 2019

Ongoing ...

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines

jasnell added 2 commits May 20, 2019 12:21
Since it will be possible to do raw quic in addition to http3,
make the alpn configurable
@jasnell jasnell force-pushed the iteration-6 branch 2 times, most recently from 0f657ab to d93aa64 Compare May 22, 2019 17:11
@jasnell
Copy link
Member Author

jasnell commented May 22, 2019

Ok... @nodejs/quic ... here's the current status:

The basic implementation is generally working with some caveats.

  1. The JS QuicStream<->Internal QuicStream data flow is not quite 100% there. It works on Windows but the end-of-output and end-of-input bits aren't working consistently across platforms. (Have I mentioned before how much I dislike our Streams API and implementation). I'll be working on making this more consistent.
  2. Error handling is largely non-existant right now.
  3. I don't believe flow control is working properly and there's definitely no backpressure implemented yet
  4. The whole mechanism around retransmission and idle timeouts needs to be completely refactored and made more efficient.
  5. Right now, when a QuicSession is told to send data, it will attempt to send stream data for every known stream even if the stream does not have any data to send. This can be made more efficient to only send from streams we know have pending data to send.
  6. We still need to implement mechanisms for cancelling streams in a reasonable way

What is working?

  1. The TLS handshake is working very well.
  2. We have keylog support that matches what just landed in master with the additional QUIC specific extensions that allow us to use wireshark to capture and decrypt traffic. 🎉 💯
  3. ALPN negotiation, SNI, and transport params are all functional
  4. QuicClientSessions will emit session ticket, id, and remote transport parameters which can be passed in when creating a new QuicClientSession to allow for connection resumption
  5. We can open unidirectional and bidirectional streams from both the client and the server.

There's still a ton of work to do here and none of the http/3 bits have been implemented at all. The plan there is to have subclasses of both the C++ and JS core classes that implement the additional http3 semantics on top.

See the test/parallel/test-quic-client-server.js test for an example of the current API surface. The docs in quic.md have not yet been updated to match but I will hopefully get to that in this iteration.

At some point, we will want to implement a prioritization mechanism. Whereas http/2 has prioritization built into the protocol, QUIC does not, and relies on the application to provide prioritization. However, HTTP/3 does include prioritization that is modeled after the http/2 model. Right now, when sending data, we simply iterate through the streams in the order they were inserted into the internal map. Once we have the prioritization scheme worked out, this will need to become a priority queue of streams that actually have data to send. I'm not currently sure if the nghttp3 library will help with this, so we'll just have to see once we get to that.

The next major task I will have on this is to refactor the JS QuicStream data flow so that it works consistently across all platforms, then I will update the documentation, followed by working on the error handling and performance.

Only extend the stream offset if we are in flowing mode on the
JS side so that the peer doesn't push too much when we're not
reading.
@jasnell
Copy link
Member Author

jasnell commented May 23, 2019

Alrighty then... I have QuicStream working consistently across Linux and Windows now and flow control should be working. Next task will be to catch the documentation up to date and start on error handling. Here soon we'll need to start working on a comprehensive set of tests.

@jasnell
Copy link
Member Author

jasnell commented May 24, 2019

Documentation updated!

jasnell and others added 12 commits May 26, 2019 12:04
Currently the following compiler error is generated with clang:
../src/node_quic_session.h:129:15: error: inline function 'node::quic::QuicSession::SetLastError' is not defined [-Werror,-Wundefined-inline]
  inline void SetLastError(
              ^
../src/node_quic_session.h:134:5: note: used here
    SetLastError(InitQuicError(family, code));
    ^
1 error generated.
make[1]: *** [/Users/danielbevenius/work/nodejs/quic/node_quic/out/Release/obj.target/libnode/src/node_quic_monitor.o] Error 1
make[1]: *** Waiting for unfinished jobs....

This commit makes moves the implemention of SetLastError into the inline
function in the header.
The `'clientHello'` and `'cert'` events emit optionally during
the TLS handshake to allow usercode the opportunity to modify
TLS handshake properties and behavior at multiple points through
the handshake process. To maximize performance, these are not
called by default.
jasnell added a commit that referenced this pull request Aug 9, 2019
jasnell added a commit that referenced this pull request Aug 9, 2019
jasnell added a commit that referenced this pull request Aug 9, 2019
jasnell added a commit that referenced this pull request Aug 9, 2019
jasnell added a commit that referenced this pull request Aug 9, 2019
jasnell added a commit that referenced this pull request Aug 19, 2019
jasnell added a commit that referenced this pull request Aug 19, 2019
Since it will be possible to do raw quic in addition to http3,
make the alpn configurable

PR-URL: #31
jasnell added a commit that referenced this pull request Aug 19, 2019
jasnell added a commit that referenced this pull request Aug 19, 2019
jasnell added a commit that referenced this pull request Aug 19, 2019
jasnell added a commit that referenced this pull request Aug 19, 2019
jasnell added a commit that referenced this pull request Aug 19, 2019
jasnell added a commit that referenced this pull request Aug 19, 2019
Only extend the stream offset if we are in flowing mode on the
JS side so that the peer doesn't push too much when we're not
reading.

PR-URL: #31
jasnell added a commit that referenced this pull request Aug 19, 2019
jasnell added a commit that referenced this pull request Aug 19, 2019
jasnell added a commit that referenced this pull request Aug 19, 2019
jasnell added a commit that referenced this pull request Aug 19, 2019
jasnell added a commit that referenced this pull request Aug 19, 2019
jasnell added a commit that referenced this pull request Aug 19, 2019
jasnell added a commit that referenced this pull request Aug 19, 2019
jasnell pushed a commit that referenced this pull request Aug 19, 2019
jasnell pushed a commit that referenced this pull request Aug 19, 2019
Currently the following compiler error is generated with clang:
../src/node_quic_session.h:129:15: error: inline function 'node::quic::QuicSession::SetLastError' is not defined [-Werror,-Wundefined-inline]
  inline void SetLastError(
              ^
../src/node_quic_session.h:134:5: note: used here
    SetLastError(InitQuicError(family, code));
    ^
1 error generated.
make[1]: *** [/Users/danielbevenius/work/nodejs/quic/node_quic/out/Release/obj.target/libnode/src/node_quic_monitor.o] Error 1
make[1]: *** Waiting for unfinished jobs....

This commit makes moves the implemention of SetLastError into the inline
function in the header.

PR-URL: #31
jasnell added a commit that referenced this pull request Aug 19, 2019
The `'clientHello'` and `'cert'` events emit optionally during
the TLS handshake to allow usercode the opportunity to modify
TLS handshake properties and behavior at multiple points through
the handshake process. To maximize performance, these are not
called by default.

PR-URL: #31
jasnell added a commit that referenced this pull request Aug 19, 2019
jasnell added a commit that referenced this pull request Aug 19, 2019
jasnell added a commit that referenced this pull request Aug 19, 2019
jasnell added a commit that referenced this pull request Aug 19, 2019
jasnell added a commit that referenced this pull request Aug 19, 2019
jasnell added a commit that referenced this pull request Aug 19, 2019
jasnell added a commit that referenced this pull request Aug 19, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants