GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — quic: implement various utilities classes to be used by the quic impl by jasnell · Pull Request #47263 · nodejs/node

Conversation

@jasnell

@jasnell jasnell commented Mar 27, 2023

Copy link
Copy Markdown
Member

In an attempt to make #44325 easier to review, I'm going to start pulling chunks of it out into separate PRs. This PR adds implementations for a number of utility classes used in the implementation. Unfortunately, breaking things up like this makes it a bit more difficult to review for correct usage since these are not yet being used, but my hope is that by splitting things up like this folks can actually give reasonable review for the individual pieces. I anticipate opening several more smaller PRs before we get to the primary chunk of the actual implementation.

This PR is split into several individual commits. It will be easiest to review each commit individually.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
lin72h, debadree25, bricss, and nikelborm reacted with thumbs up emoji lin72h, bricss, and debadree25 reacted with hooray emoji lin72h, aduh95, debadree25, bricss, and guillaumekh reacted with heart emoji lin72h, bricss, and debadree25 reacted with rocket emoji bricss, lin72h, and debadree25 reacted with eyes emoji
All reactions
  • 👍 4 reactions
  • 🎉 3 reactions
  • ❤️ 5 reactions
  • 🚀 3 reactions
  • 👀 3 reactions
@nodejs-github-bot

nodejs-github-bot commented Mar 27, 2023

Copy link
Copy Markdown
Collaborator

Review requested:

  • @nodejs/gyp
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs involving general changes in the lib/ or src/ directories. needs-ci PRs that need a full CI run. labels Mar 27, 2023
@jasnell jasnell mentioned this pull request Mar 27, 2023
src: quic #44325
Closed
@jasnell

jasnell commented Mar 27, 2023

Copy link
Copy Markdown
Member Author

/cc @bnoordhuis @addaleax @Qard @tniessen @mcollina @anonrig
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@jasnell jasnell changed the title quic: add the CID implementation quic: implement various utilities classes to be used by the quic impl Mar 27, 2023
@jasnell jasnell added dont-land-on-v14.x quic Issues and PRs related to the QUIC transport implementation. labels Mar 27, 2023
VoltrexKeyva
VoltrexKeyva reviewed Mar 27, 2023
View reviewed changes
Comment thread src/quic/cid.h Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread src/quic/cid.h Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread src/quic/data.cc Resolved

Uh oh!

There was an error while loading. Please reload this page.
tniessen
tniessen reviewed Mar 27, 2023
View reviewed changes
Comment thread src/quic/cid.h Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread src/quic/data.cc Resolved

Uh oh!

There was an error while loading. Please reload this page.
jasnell
jasnell commented Mar 27, 2023
View reviewed changes
Comment thread src/quic/preferredaddress.h Resolved

Uh oh!

There was an error while loading. Please reload this page.
anonrig
anonrig reviewed Mar 27, 2023
View reviewed changes
Comment thread src/quic/cid.cc Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread src/quic/cid.h Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread src/quic/data.cc Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread src/quic/preferredaddress.cc Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread src/quic/preferredaddress.cc Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread src/quic/preferredaddress.cc Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread src/quic/preferredaddress.h Resolved

Uh oh!

There was an error while loading. Please reload this page.
anonrig
anonrig approved these changes Mar 27, 2023
View reviewed changes
@jasnell jasnell added the request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. label Mar 27, 2023
jasnell added 7 commits March 27, 2023 13:15
@jasnell
quic: add the CID implementation
9d95723
@jasnell
quic: add the PreferredAddress implementation
9c6c3b0
@jasnell
quic: add Path and PathStorage implementations
d092604
@jasnell
quic: add Store implementation
e762f99 
Store is a utility class that abstracts working with BackingStore
that will be used in the quic impl
@jasnell
quic: add QuicError implementation
a2b787f
@jasnell
quic: fixup lining issues in multiple files
6e1b1b8
@jasnell
quic: apply format-cpp to multiple files
abdf755
@jasnell
jasnell force-pushed the quic-part-1 branch from 14e4d17 to abdf755 Compare March 27, 2023 20:15
@github-actions github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. label Mar 27, 2023
@nodejs-github-bot

This comment was marked as outdated.

Sign in to view
@jasnell
quic: fixup silly compile errors
35556f8
@nodejs-github-bot

This comment was marked as outdated.

Sign in to view
@nodejs-github-bot

This comment was marked as outdated.

Sign in to view
@nodejs-github-bot

This comment was marked as outdated.

Sign in to view
@nodejs-github-bot

This comment was marked as outdated.

Sign in to view
@nodejs-github-bot

nodejs-github-bot commented Mar 28, 2023 •
edited by jasnell
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/50635/ 💛
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@jasnell
quic: add shared openssl build guard
7d8e349
@jasnell
jasnell force-pushed the quic-part-1 branch from ed6f365 to 7d8e349 Compare March 28, 2023 03:28
Qard
Qard approved these changes Mar 28, 2023
View reviewed changes

@Qard Qard left a comment

Copy link
Copy Markdown
Member

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.

LGTM with some suggestions. One inline and another is: I see a bunch of use of both std::optional and Maybe. Is there a reason we can't (or should not) just use Maybe everywhere? 🤔

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread src/quic/cid.cc Resolved

Uh oh!

There was an error while loading. Please reload this page.
@RaisinTen

RaisinTen commented Mar 28, 2023

Copy link
Copy Markdown
Member

Is there a reason we can't (or should not) just use Maybe everywhere?

A Maybe should be used if a function needs to propagate info about whether a V8 exception is being thrown. Using Maybe instead of optional would make it look like an exception has been thrown when a function returns a nullopt but that's not true for the functions that are using optionals in this PR.
jasnell reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@jasnell

jasnell commented Mar 28, 2023

Copy link
Copy Markdown
Member Author

Definitely agree that having both v8::Maybe and std::optional is annoying but the semantics for v8::Maybe are pretty clear. It's not what I'd want ideally but I think the current code is the currently better approach.
tniessen reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@jasnell jasnell added author ready PRs with CI started, the required approvals, and no outstanding review comments. and removed needs-ci PRs that need a full CI run. labels Mar 28, 2023
@nodejs-github-bot

This comment was marked as outdated.

Sign in to view
@jasnell

jasnell commented Mar 28, 2023 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member Author

CI passed. Made one additional edit as suggested by @Qard so running CI again. The plan is to squash all the commits down into one when this lands. Started preparing the second PR. lol...

update: sigh, forgot to actually push the new commit.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@jasnell
fixup! quic: add the CID implementation
dde6f5d
@nodejs-github-bot

This comment was marked as outdated.

Sign in to view
Qard
Qard approved these changes Mar 28, 2023
View reviewed changes
@nodejs-github-bot

This comment was marked as outdated.

Sign in to view
tniessen
tniessen approved these changes Mar 28, 2023
View reviewed changes
@nodejs-github-bot

This comment was marked as outdated.

Sign in to view
@Qard

Qard commented Mar 28, 2023

Copy link
Copy Markdown
Member

Generally speaking, it's more MaybeLocal that the Nothing could represent a pending exception. That's usually not the case with Maybe. It's quite often used in V8 as a direct replacement for std::optional only. I'm fine if we want to keep V8 things distinct from our own stuff though. :)
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@nodejs-github-bot

This comment was marked as outdated.

Sign in to view
@nodejs-github-bot

nodejs-github-bot commented Mar 28, 2023 •
edited by jasnell
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/50655/ 💛 finally!
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
jasnell added a commit that referenced this pull request Mar 29, 2023
@jasnell
quic: add multiple internal utilities
09a4bb1 
* add the CID implementation
* add the PreferredAddress implementation
* add Path and PathStorage implementations
* add Store implementation
* add QuicError implementation

PR-URL: #47263
Reviewed-By: Yagiz Nizipli <[email protected]>
Reviewed-By: Stephen Belanger <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
@jasnell

jasnell commented Mar 29, 2023

Copy link
Copy Markdown
Member Author

Landed in 09a4bb1
Qard, prostreyd, Spade-and-Archer, coolguy284, jwerle, richardeschloss, Remzi1993, and derhuerst reacted with hooray emoji Qard, prostreyd, jwerle, and metcoder95 reacted with heart emoji Qard, wmelton, Roman-Simik, and jwerle reacted with rocket emoji
All reactions
  • 🎉 8 reactions
  • ❤️ 4 reactions
  • 🚀 4 reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@jasnell jasnell closed this Mar 29, 2023
This was referenced Mar 30, 2023
CI Reliability 2023-03-30 nodejs/reliability#534
Open
CI Reliability 2023-03-31 nodejs/reliability#535
Open
This was referenced Apr 3, 2023
2023-04-18, Version 20.0.0 (Current) #47381
Merged
quic: do not dereference shared_ptr after move #47294
Closed
@derhuerst derhuerst mentioned this pull request Mar 4, 2026
QUIC support #23064
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@Qard Qard Qard approved these changes
@anonrig anonrig anonrig approved these changes
@tniessen tniessen tniessen approved these changes
+1 more reviewer
@VoltrexKeyva VoltrexKeyva VoltrexKeyva left review comments
Reviewers whose approvals may not affect merge requirements

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

author ready PRs with CI started, the required approvals, and no outstanding review comments. c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs involving general changes in the lib/ or src/ directories. quic Issues and PRs related to the QUIC transport implementation.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

7 participants

@jasnell @nodejs-github-bot @RaisinTen @Qard @anonrig @tniessen @VoltrexKeyva
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.