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.

Sign up for GitHub

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

Jump to bottom

quic: add quic internalBinding, refine Endpoint, add types #51112

Closed

jasnell wants to merge 1 commit into nodejs:main from jasnell:quic-part-8

Member

jasnell commented Dec 11, 2023

More quic implementation details. Number of fixes, refinements, continued implementation.

jasnell requested review from Qard and anonrig

December 11, 2023 03:08

Collaborator

nodejs-github-bot commented Dec 11, 2023

Review requested:

@nodejs/gyp

nodejs-github-bot added c++ lib / src needs-ci labels

jasnell force-pushed the quic-part-8 branch 2 times, most recently from ce6c379 to 5c7b1e5 Compare

December 11, 2023 03:24

jasnell added the quic label

This comment was marked as outdated.

Sign in to view

jasnell requested a review from tniessen

December 11, 2023 03:28

jasnell force-pushed the quic-part-8 branch 4 times, most recently from f2c34bb to ba0f9ec Compare

December 11, 2023 04:11

This comment was marked as outdated.

Sign in to view

Qard approved these changes

View reviewed changes

Member

Qard left a comment

Some nits but otherwise LGTM.

src/quic/endpoint.h Outdated Show resolved Hide resolved

test/parallel/test-quic-internal-setcallbacks.js Outdated Show resolved Hide resolved

anonrig approved these changes

View reviewed changes

tniessen approved these changes

View reviewed changes

jasnell force-pushed the quic-part-8 branch 2 times, most recently from c3d26ca to 183df09 Compare

December 22, 2023 15:55

This comment was marked as outdated.

Sign in to view

jasnell force-pushed the quic-part-8 branch from 183df09 to 15cb9dc Compare

December 22, 2023 16:08

Member Author

jasnell commented Dec 22, 2023

At least one of the tests are failing on multiple systems in CI.

jasnell force-pushed the quic-part-8 branch from 15cb9dc to 8f6a634 Compare

December 23, 2023 15:19

jasnell added the request-ci label

github-actions bot removed the request-ci label

This comment was marked as outdated.

Sign in to view

jasnell force-pushed the quic-part-8 branch from 8f6a634 to f112efb Compare

December 23, 2023 15:25

This comment was marked as outdated.

Sign in to view

This comment was marked as outdated.

Sign in to view

jasnell force-pushed the quic-part-8 branch from 8318250 to 34053ea Compare

December 24, 2023 04:20

This comment was marked as outdated.

Sign in to view

Member

targos commented Dec 24, 2023 •

edited

Loading

Minimum example to trigger the segfault:

'use strict';

require('../common');

const { internalBinding } = require('internal/test/binding');
const quic = internalBinding('quic');

quic.createEndpoint({});

./node_g --expose-internals test/parallel/test-quic-internal-endpoint-options.js

Note that it only happens if we require('../common').

This comment was marked as outdated.

Sign in to view

Member Author

jasnell commented Dec 24, 2023

@targos ... try as I might I've been unable to reproduce the failure locally on four separate machines (windows, linux or macos). I'm likely going to need some help tracking down exactly what the issue is as it's not clear by any stretch what could be causing the segfault. Also, I tried doing an asan build but that appears to not be working? The asan build consistently fails at the mksnapshotstep (/cc @joyeecheung )

Member

targos commented Dec 25, 2023

Happy to help however I can. It reproduces consistently on my mac.

This comment was marked as outdated.

Sign in to view

jasnell force-pushed the quic-part-8 branch 2 times, most recently from f3a2757 to 320b300 Compare

December 25, 2023 22:09

Member Author

jasnell commented Dec 25, 2023

Ok, think I got the segfault figured out. Looks like a couple remaining issues on windows.

This comment was marked as outdated.

Sign in to view

jasnell force-pushed the quic-part-8 branch from 320b300 to 231d15a Compare

December 26, 2023 01:58

This comment was marked as outdated.

Sign in to view

This comment was marked as outdated.

Sign in to view

jasnell force-pushed the quic-part-8 branch from 231d15a to ded203e Compare

December 26, 2023 17:22

This comment was marked as outdated.

Sign in to view

This comment was marked as outdated.

Sign in to view


          quic: add quic internalBinding, refine Endpoint, add types

0b42596

jasnell force-pushed the quic-part-8 branch from ded203e to 0b42596 Compare

December 26, 2023 20:20

This comment was marked as outdated.

Sign in to view

This comment was marked as outdated.

Sign in to view

jasnell mentioned this pull request

deps: update ngtcp2 and nghttp3, update quic impls #51291

Closed

Collaborator

nodejs-github-bot commented Dec 27, 2023 •

edited by jasnell

Loading

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

jasnell added a commit that referenced this pull request


          quic: add quic internalBinding, refine Endpoint, add types

c366422

PR-URL: #51112
Reviewed-By: Stephen Belanger <admin@stephenbelanger.com>
Reviewed-By: Yagiz Nizipli <yagiz.nizipli@sentry.io>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>

Member Author

jasnell commented Dec 27, 2023

Landed in c366422

jasnell closed this

RafaelGSS pushed a commit that referenced this pull request


          quic: add quic internalBinding, refine Endpoint, add types

5b32e21

PR-URL: #51112
Reviewed-By: Stephen Belanger <admin@stephenbelanger.com>
Reviewed-By: Yagiz Nizipli <yagiz.nizipli@sentry.io>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>

RafaelGSS mentioned this pull request

v21.6.0 proposal #51342

Merged

marco-ippolito added the dont-land-on-v20.x label

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

c++ dont-land-on-v20.x lib / src needs-ci quic