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: refactor ocsp handling #34498

Closed
wants to merge 3 commits into from
Closed

Conversation

jasnell
Copy link
Member

@jasnell jasnell commented Jul 23, 2020

First two commits are cleanup.

Third commit refactors the OCSP handling, converting it from a callback-oriented event to a configuration-based async function, which makes a lot more sense given that it's only ever called once at a very specific point in the QuicSession lifecycle.

Fourth commit removes the 'clientHello' event. Use cases supporting the event are tenuous at best and do not justify the additional machinery necessary for it to work.

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 jasnell requested a review from a team July 23, 2020 20:19
@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 related to general changes in the lib or src directory. labels Jul 23, 2020
@nodejs-github-bot

This comment has been minimized.

@nodejs-github-bot

This comment has been minimized.

@nodejs-github-bot

This comment has been minimized.

@nodejs-github-bot

This comment has been minimized.

@nodejs-github-bot

This comment has been minimized.

@jasnell

This comment has been minimized.

@jasnell jasnell marked this pull request as draft July 24, 2020 00:49
@jasnell jasnell changed the title quic: refactor ocsp handling and remove clientHello event quic: refactor ocsp handling Jul 27, 2020
@jasnell jasnell marked this pull request as ready for review July 27, 2020 16:47
@jasnell
Copy link
Member Author

jasnell commented Jul 27, 2020

Ping @nodejs/quic ... this is ready for review. Limited the PR to just the OCSP handling for now. Still working through the issues around client hello, SNI, and SecureContext selection at the start of a QuicSession... will move those changes into a separate PR

@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Jul 27, 2020

@@ -1858,7 +1858,6 @@ class QuicSession extends EventEmitter {

if (!this[kHandshakePost]()) {
if (typeof state.handshakeCompletePromiseReject === 'function') {
// TODO(@jasnell): Proper error
state.handshakeCompletePromiseReject(
new ERR_OPERATION_FAILED('Handshake failed'));
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can I disagree about these TODOs being irrelevant now? ERR_OPERATION_FAILED is too generic imo, because an operation failing is just about the definition of an error :)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I know we've had a conversation about this before when you mentioned that our errors are too specific in cases. Struggling to find the right balance. Do you have a more concrete suggestion?

@nodejs-github-bot
Copy link
Collaborator

@jasnell jasnell added quic Issues and PRs related to the QUIC implementation / HTTP/3. dont-land-on-v14.x labels Jul 27, 2020
@jasnell
Copy link
Member Author

jasnell commented Jul 27, 2020

Landed in 62198d2...1f94b89

@jasnell jasnell closed this Jul 27, 2020
jasnell added a commit that referenced this pull request Jul 27, 2020
PR-URL: #34498
Reviewed-By: Anna Henningsen <anna@addaleax.net>
jasnell added a commit that referenced this pull request Jul 27, 2020
PR-URL: #34498
Reviewed-By: Anna Henningsen <anna@addaleax.net>
jasnell added a commit that referenced this pull request Jul 27, 2020
PR-URL: #34498
Reviewed-By: Anna Henningsen <anna@addaleax.net>
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++. lib / src Issues and PRs related to general changes in the lib or src directory. quic Issues and PRs related to the QUIC implementation / HTTP/3.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants