quic: refactor clientHello #34541

jasnell · 2020-07-28T17:49:06Z

First commit here is from #34533 which needs to land first.

Second commit here is the important one in this PR. This refactors the 'clientHello' event into an async function passed as an quicsocket.listen() option. This function is only ever called once at a very specific point in the QuicServerSession lifecycle, using it as an event is unnecessary. The commit changes the way it works. If appropriate to do so, user code may return a new SecureContext from the function (previously that was done in the OCSP function, which really didn't make sense).

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

nodejs-github-bot · 2020-07-28T17:50:06Z

CI: https://ci.nodejs.org/job/node-test-pull-request/32525/
QUIC CI: https://ci.nodejs.org/job/node-test-commit/39882/

nodejs-github-bot · 2020-07-28T18:38:22Z

CI: https://ci.nodejs.org/job/node-test-pull-request/32527/

richardlau · 2020-07-28T19:16:26Z

The actions failures are new warnings (treated as errors):

2020-07-28T17:59:21.1359496Z ../src/quic/node_quic_session.cc: In function ‘void node::quic::{anonymous}::QuicSessionOnCertDone(const v8::FunctionCallbackInfo<v8::Value>&)’:
2020-07-28T17:59:21.1455906Z ../src/quic/node_quic_session.cc:2732:16: error: unused variable ‘env’ [-Werror=unused-variable]
2020-07-28T17:59:21.1456744Z    Environment* env = Environment::GetCurrent(args);
2020-07-28T17:59:21.1457123Z                 ^~~
2020-07-28T17:59:21.9743160Z ../src/quic/node_quic_session.cc: In member function ‘virtual void node::quic::JSQuicSessionListener::OnClientHello(const char*, const char*)’:
2020-07-28T17:59:21.9744621Z ../src/quic/node_quic_session.cc:399:55: error: ignoring return value of ‘bool v8::MaybeLocal<T>::ToLocal(v8::Local<S>*) const [with S = v8::Array; T = v8::Array]’, declared with attribute warn_unused_result [-Werror=unused-result]
2020-07-28T17:59:21.9745989Z    session()->crypto_context()->hello_ciphers().ToLocal(&ciphers);
2020-07-28T17:59:21.9746893Z    ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~
2020-07-28T17:59:21.9748732Z ../src/quic/node_quic_session.cc:402:54: error: ignoring return value of ‘bool v8::MaybeLocal<T>::ToLocal(v8::Local<S>*) const [with S = v8::String; T = v8::String]’, declared with attribute warn_unused_result [-Werror=unused-result]
2020-07-28T17:59:21.9749778Z      String::NewFromUtf8(env->isolate(), alpn).ToLocal(&alpn_string);
2020-07-28T17:59:21.9749960Z      ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~
2020-07-28T17:59:21.9750767Z ../src/quic/node_quic_session.cc:405:61: error: ignoring return value of ‘bool v8::MaybeLocal<T>::ToLocal(v8::Local<S>*) const [with S = v8::String; T = v8::String]’, declared with attribute warn_unused_result [-Werror=unused-result]
2020-07-28T17:59:21.9751274Z      String::NewFromUtf8(env->isolate(), server_name).ToLocal(&servername);
2020-07-28T17:59:21.9751434Z      ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~
2020-07-28T17:59:21.9807567Z ../src/quic/node_quic_session.cc: In member function ‘virtual void node::quic::JSQuicSessionListener::OnHandshakeCompleted()’:
2020-07-28T17:59:21.9808402Z ../src/quic/node_quic_session.cc:581:55: error: ignoring return value of ‘bool v8::MaybeLocal<T>::ToLocal(v8::Local<S>*) const [with S = v8::Value; T = v8::Value]’, declared with attribute warn_unused_result [-Werror=unused-result]
2020-07-28T17:59:21.9808617Z      crypto::GetValidationErrorReason(env, err).ToLocal(&validationErrorReason);
2020-07-28T17:59:21.9808753Z      ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~
2020-07-28T17:59:21.9809720Z ../src/quic/node_quic_session.cc:582:53: error: ignoring return value of ‘bool v8::MaybeLocal<T>::ToLocal(v8::Local<S>*) const [with S = v8::Value; T = v8::Value]’, declared with attribute warn_unused_result [-Werror=unused-result]
2020-07-28T17:59:21.9810060Z      crypto::GetValidationErrorCode(env, err).ToLocal(&validationErrorCode);
2020-07-28T17:59:21.9810235Z      ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~
2020-07-28T17:59:26.2727155Z cc1plus: all warnings being treated as errors

jasnell · 2020-07-28T20:54:53Z

CI is good on this!

src/quic/node_quic_session.cc

jasnell · 2020-07-29T18:30:00Z

@addaleax, please take another look when you have the opportunity to do so. I've added a new QuicCallbackScope modeled after CallbackScope but it ensures that the QuicSession is destroyed if an error is thrown, allowing an attempt to be made to send a final CONNECTION_CLOSE to the peer.

lib/internal/quic/core.js

test/parallel/test-quic-client-server.js

src/quic/node_quic_session.cc

Refactor the `'clientHello'` event into a `clientHelloHandler` configuration option and async function. Remove the addContext API as it's not needed.

Alternative to `CallbackScope` that handles destroying the `QuicSession` in the try_catch cleanup.

jasnell · 2020-07-31T18:10:55Z

CI: https://ci.nodejs.org/job/node-test-pull-request/32578/
QUIC CI: https://ci.nodejs.org/job/node-test-commit/39944/

Resumed Quic CI: https://ci.nodejs.org/job/node-test-commit/39945/

jasnell · 2020-07-31T22:05:17Z

Regular CI and QUIC CI are good to go on this

Refactor the `'clientHello'` event into a `clientHelloHandler` configuration option and async function. Remove the addContext API as it's not needed. PR-URL: #34541 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Jiawen Geng <technicalcute@gmail.com>

Alternative to `CallbackScope` that handles destroying the `QuicSession` in the try_catch cleanup. PR-URL: #34541 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Jiawen Geng <technicalcute@gmail.com>

PR-URL: #34541 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Jiawen Geng <technicalcute@gmail.com>

gengjiawen · 2020-08-03T12:03:51Z

Landed in e5dacc2...6e65f26

jasnell requested a review from as a code owner Jul 28, 2020

nodejs-github-bot added c++ lib / src labels Jul 28, 2020

jasnell added dont-land-on-v14.x quic labels Jul 28, 2020

jasnell changed the title ~~quic: use OpenSSL built-in cert and hostname validation~~ quic: refactor clientHello Jul 28, 2020

richardlau added the dont-land-on-v12.x label Jul 28, 2020

github-actions bot mentioned this pull request Jul 29, 2020

CI Relability 2020-07-29 JaneaSystems/nodejs-reliability#52

Open

6 tasks

addaleax approved these changes Jul 29, 2020

View changes

src/quic/node_quic_session.cc Outdated Show resolved Hide resolved

jasnell force-pushed the quic-cleanups-10 branch from b8b6ce0 to 3f902ed Compare Jul 29, 2020

github-actions bot mentioned this pull request Jul 29, 2020

CI Reliability 2020-07-29 nodejs/reliability#27

Open

7 tasks

jasnell force-pushed the quic-cleanups-10 branch from 2342fb7 to b95bf9d Compare Jul 29, 2020

github-actions bot mentioned this pull request Jul 30, 2020

CI Relability 2020-07-30 JaneaSystems/nodejs-reliability#53

Open

7 tasks

github-actions bot mentioned this pull request Jul 30, 2020

CI Reliability 2020-07-30 nodejs/reliability#28

Open

7 tasks

lundibundi reviewed Jul 30, 2020

View changes

lib/internal/quic/core.js Outdated Show resolved Hide resolved

test/parallel/test-quic-client-server.js Show resolved Hide resolved

lundibundi reviewed Jul 30, 2020

View changes

src/quic/node_quic_session.cc Show resolved Hide resolved

github-actions bot mentioned this pull request Jul 31, 2020

CI Relability 2020-07-31 JaneaSystems/nodejs-reliability#54

Open

8 tasks

github-actions bot mentioned this pull request Jul 31, 2020

CI Reliability 2020-07-31 nodejs/reliability#29

Open

8 tasks

jasnell added 3 commits Jul 31, 2020

quic: refactor clientHello

aa00ee0

Refactor the `'clientHello'` event into a `clientHelloHandler` configuration option and async function. Remove the addContext API as it's not needed.

quic: introduce QuicCallbackScope

d2da015

Alternative to `CallbackScope` that handles destroying the `QuicSession` in the try_catch cleanup.

quic: use QuicCallbackScope consistently for QuicSession

ff8677f

jasnell force-pushed the quic-cleanups-10 branch from 3b7a7ee to ff8677f Compare Jul 31, 2020

jasnell added the author ready label Jul 31, 2020

jasnell mentioned this pull request Jul 31, 2020

test-macOS-with-quic broken #34587

Closed

github-actions bot mentioned this pull request Aug 1, 2020

CI Relability 2020-08-01 JaneaSystems/nodejs-reliability#55

Open

10 tasks

github-actions bot mentioned this pull request Aug 1, 2020

CI Reliability 2020-08-01 nodejs/reliability#30

Open

10 tasks

github-actions bot mentioned this pull request Aug 2, 2020

CI Relability 2020-08-02 JaneaSystems/nodejs-reliability#56

Open

11 tasks

github-actions bot mentioned this pull request Aug 2, 2020

CI Reliability 2020-08-02 nodejs/reliability#31

Open

11 tasks

github-actions bot mentioned this pull request Aug 3, 2020

CI Relability 2020-08-03 JaneaSystems/nodejs-reliability#57

Open

12 tasks

github-actions bot mentioned this pull request Aug 3, 2020

CI Reliability 2020-08-03 nodejs/reliability#32

Open

12 tasks

gengjiawen approved these changes Aug 3, 2020

View changes

gengjiawen added a commit that referenced this issue Aug 3, 2020

quic: use QuicCallbackScope consistently for QuicSession

6e65f26

PR-URL: #34541 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Jiawen Geng <technicalcute@gmail.com>

gengjiawen closed this Aug 3, 2020

github-actions bot mentioned this pull request Aug 4, 2020

CI Relability 2020-08-04 JaneaSystems/nodejs-reliability#58

Open

12 tasks

github-actions bot mentioned this pull request Aug 4, 2020

CI Reliability 2020-08-04 nodejs/reliability#33

Open

13 tasks

nodejs / node Public

quic: refactor clientHello #34541

quic: refactor clientHello #34541

jasnell commented Jul 28, 2020

nodejs-github-bot commented Jul 28, 2020 •

edited by jasnell

nodejs-github-bot commented Jul 28, 2020

richardlau commented Jul 28, 2020

jasnell commented Jul 28, 2020

jasnell commented Jul 29, 2020

jasnell commented Jul 31, 2020 •

edited

jasnell commented Jul 31, 2020

gengjiawen commented Aug 3, 2020

nodejs / node Public

quic: refactor clientHello #34541

quic: refactor clientHello #34541

Conversation

jasnell commented Jul 28, 2020

Checklist

nodejs-github-bot commented Jul 28, 2020 • edited by jasnell

nodejs-github-bot commented Jul 28, 2020

richardlau commented Jul 28, 2020

jasnell commented Jul 28, 2020

jasnell commented Jul 29, 2020

jasnell commented Jul 31, 2020 • edited

jasnell commented Jul 31, 2020

gengjiawen commented Aug 3, 2020

nodejs-github-bot commented Jul 28, 2020 •

edited by jasnell

jasnell commented Jul 31, 2020 •

edited