Skip to content
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: another round of refactorings #34247

Closed
wants to merge 16 commits into from

Conversation

jasnell
Copy link
Member

@jasnell jasnell commented Jul 7, 2020

This is a WIP, more will be added:

Commits:

  • quic: additional minor cleanups in node_quic_session.h

    Minor miscellaneous cleanups

  • quic: remove unnecessary bool conversion

    Self-explanatory

  • quic: handle errors thrown / rejections in the session event

  • quic: refactor/improve error handling for busy event

  • quic: add tests confirming error handling for QuicSocket close event

  • quic: refactor/improve QuicSocket ready event handling

  • quic: refactor/improve QuicSocket ready event handling

    Starting to improve/refactor/verify error handling on events

  • quic: use Number() instead of bigint for QuicSocket stats

  • quic: unref timers again

    Fix a bug

  • quic: use getter/setting for stateless reset toggle

    Improve API ergonomics

  • quic: cleanup QuicSocketFlags, used shared state struct

    Use AliasedStruct for QuicSocket shared state, cleanup no longer used state flags

  • quic: proper custom inspect for QuicEndpoint/QuicSocket/QuicSession/QuicStream

    Self explanatory

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 nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. quic Issues and PRs related to the QUIC implementation / HTTP/3. labels Jul 7, 2020
@jasnell jasnell added the experimental Issues and PRs related to experimental features. label Jul 7, 2020
Errors thrown within the session event handler will be handled
by destroying the session (allowing a proper connection close
to be sent to the client peer). They will not crash the parent
QuicSocket by default. Instead, a `'sessionError'` event will
be emitted, allowing the error to be logged or handled.
@jasnell

This comment has been minimized.

@addaleax

This comment has been minimized.

@jasnell

This comment has been minimized.

@addaleax

This comment has been minimized.

@addaleax

This comment has been minimized.

@jasnell

This comment has been minimized.

@addaleax

This comment has been minimized.

0f97d60 accidentally removed this.

Refs: nodejs#34186
@jasnell

This comment has been minimized.

@jasnell jasnell force-pushed the quic-cleanups-4 branch 2 times, most recently from c767d37 to f044069 Compare July 8, 2020 16:06
Some of the flags were no longer being used.

Switched to use an AliasedStruct for shared state to avoid
extraneous expensive JS=>C++ calls.

Removed unused QuicSocket option
@jasnell jasnell force-pushed the quic-cleanups-4 branch 2 times, most recently from e5cea72 to 2a61277 Compare July 8, 2020 17:05
@jasnell jasnell marked this pull request as ready for review July 8, 2020 17:07
@jasnell jasnell requested a review from a team as a code owner July 8, 2020 17:07
@jasnell
Copy link
Member Author

jasnell commented Jul 8, 2020

Ping @nodejs/quic... this is ready for review

@jasnell
Copy link
Member Author

jasnell commented Jul 8, 2020

Both CI's are good.

jasnell added a commit that referenced this pull request Jul 9, 2020
PR-URL: #34247
Reviewed-By: Anna Henningsen <anna@addaleax.net>
jasnell added a commit that referenced this pull request Jul 9, 2020
The argument will always be a boolean already

PR-URL: #34247
Reviewed-By: Anna Henningsen <anna@addaleax.net>
jasnell added a commit that referenced this pull request Jul 9, 2020
Errors thrown within the session event handler will be handled
by destroying the session (allowing a proper connection close
to be sent to the client peer). They will not crash the parent
QuicSocket by default. Instead, a `'sessionError'` event will
be emitted, allowing the error to be logged or handled.

PR-URL: #34247
Reviewed-By: Anna Henningsen <anna@addaleax.net>
jasnell added a commit that referenced this pull request Jul 9, 2020
Also, change setServerBusy into a setter

PR-URL: #34247
Reviewed-By: Anna Henningsen <anna@addaleax.net>
jasnell added a commit that referenced this pull request Jul 9, 2020
PR-URL: #34247
Reviewed-By: Anna Henningsen <anna@addaleax.net>
jasnell added a commit that referenced this pull request Jul 9, 2020
PR-URL: #34247
Reviewed-By: Anna Henningsen <anna@addaleax.net>
jasnell added a commit that referenced this pull request Jul 9, 2020
PR-URL: #34247
Reviewed-By: Anna Henningsen <anna@addaleax.net>
jasnell added a commit that referenced this pull request Jul 9, 2020
PR-URL: #34247
Reviewed-By: Anna Henningsen <anna@addaleax.net>
jasnell pushed a commit that referenced this pull request Jul 9, 2020
0f97d60 accidentally removed this.

Refs: #34186

PR-URL: #34247
Reviewed-By: James M Snell <jasnell@gmail.com>
jasnell added a commit that referenced this pull request Jul 9, 2020
PR-URL: #34247
Reviewed-By: Anna Henningsen <anna@addaleax.net>
jasnell added a commit that referenced this pull request Jul 9, 2020
Some of the flags were no longer being used.

Switched to use an AliasedStruct for shared state to avoid
extraneous expensive JS=>C++ calls.

Removed unused QuicSocket option

PR-URL: #34247
Reviewed-By: Anna Henningsen <anna@addaleax.net>
jasnell added a commit that referenced this pull request Jul 9, 2020
PR-URL: #34247
Reviewed-By: Anna Henningsen <anna@addaleax.net>
jasnell added a commit that referenced this pull request Jul 9, 2020
PR-URL: #34247
Reviewed-By: Anna Henningsen <anna@addaleax.net>
jasnell added a commit that referenced this pull request Jul 9, 2020
PR-URL: #34247
Reviewed-By: Anna Henningsen <anna@addaleax.net>
jasnell added a commit that referenced this pull request Jul 9, 2020
PR-URL: #34247
Reviewed-By: Anna Henningsen <anna@addaleax.net>
jasnell added a commit that referenced this pull request Jul 9, 2020
PR-URL: #34247
Reviewed-By: Anna Henningsen <anna@addaleax.net>
@jasnell
Copy link
Member Author

jasnell commented Jul 9, 2020

Landed in e8f5745...26493c0

@MylesBorins
Copy link
Member

Marked as "backport-requested" because I do not believe quic is on 14.x yet. Should we change the label to "dont-land"?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c++ Issues and PRs that require attention from people who are familiar with C++. experimental Issues and PRs related to experimental features. quic Issues and PRs related to the QUIC implementation / HTTP/3.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants