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.

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: multiple TODO cleanups #34655

Closed
wants to merge 3 commits into from
Closed

quic: multiple TODO cleanups #34655

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
3c7cbef
quic: resolve minor TODO in QuicSocket
jasnell Aug 6, 2020
b82bf12
quic: resolve some minor TODOs
jasnell Aug 7, 2020
a893264
quic: limit push check to http/3
jasnell Aug 7, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
18 changes: 8 additions & 10 deletions lib/internal/quic/core.js
View file
Open in desktop
Expand Up @@ -517,7 +517,6 @@ function createSecureContext(options, init_cb) {
const sc_options = validateCreateSecureContextOptions(options);
const { groups, earlyData } = sc_options;
const sc = _createSecureContext(sc_options);
// TODO(@jasnell): Determine if it's really necessary to pass in groups here.
init_cb(sc.context, groups, earlyData);
return sc;
}
Expand Down Expand Up @@ -2702,7 +2701,6 @@ class QuicStream extends Duplex {
}

[kInspect](depth, options) {
// TODO(@jasnell): Proper custom inspect implementation
const direction = this.bidirectional ? 'bidirectional' : 'unidirectional';
const initiated = this.serverInitiated ? 'server' : 'client';
return customInspect(this, {
Expand Down Expand Up @@ -2944,14 +2942,14 @@ class QuicStream extends Duplex {

validateObject(headers, 'headers');

// Push streams are only supported on QUIC servers, and
// only if the original stream is bidirectional.
// TODO(@jasnell): This is really an http/3 specific
// requirement so if we end up later with another
// QUIC application protocol that has a similar
// notion of push streams without this restriction,
// then we'll need to check alpn value here also.
if (!this.clientInitiated && !this.bidirectional) {
// This is a small performance optimization for http/3,
// where push streams are only supported for client
// initiated, bidirectional streams. For any other alpn,
// we'll make the attempt to push and handle the failure
// after.
if (!this.clientInitiated &&
!this.bidirectional &&
this.session.alpnProtocol === 'h3-29') {
jasnell marked this conversation as resolved.
Show resolved Hide resolved
throw new ERR_INVALID_STATE(
'Push streams are only supported on client-initiated, ' +
'bidirectional streams');
Expand Down
9 changes: 5 additions & 4 deletions src/quic/node_quic_session.cc
View file
Open in desktop
Expand Up @@ -3378,8 +3378,10 @@ int QuicSession::OnStreamReset(
// sensitivity of PATH_CHALLENGE operations (an attacker
// could use a compromised PATH_CHALLENGE to trick an endpoint
// into redirecting traffic).
// TODO(@jasnell): In the future, we'll want to explore whether
// we want to handle the different cases of ngtcp2_rand_ctx
//
// The ngtcp2_rand_ctx tells us what the random data is used for.
// Currently, there is only one use. In the future, we'll want to
// explore whether we want to handle the different cases uses.
int QuicSession::OnRand(
ngtcp2_conn* conn,
uint8_t* dest,
Expand Down Expand Up @@ -3744,8 +3746,7 @@ void QuicSessionSilentClose(const FunctionCallbackInfo<Value>& args) {
session->Close(QuicSessionListener::SESSION_CLOSE_FLAG_SILENT);
}

// TODO(addaleax): This is a temporary solution for testing and should be
// removed later.
// This is used purely for testing.
void QuicSessionRemoveFromSocket(const FunctionCallbackInfo<Value>& args) {
QuicSession* session;
ASSIGN_OR_RETURN_UNWRAP(&session, args.Holder());
Expand Down
11 changes: 2 additions & 9 deletions src/quic/node_quic_socket.cc
View file
Open in desktop
Expand Up @@ -472,14 +472,7 @@ void QuicSocket::OnReceive(
QuicCID dcid(pdcid, pdcidlen);
QuicCID scid(pscid, pscidlen);

// TODO(@jasnell): It would be fantastic if Debug() could be
// modified to accept objects with a ToString-like capability
// similar to what we can do with TraceEvents... that would
// allow us to pass the QuicCID directly to Debug and have it
// converted to hex only if the category is enabled so we can
// skip committing resources here.
std::string dcid_hex = dcid.ToString();
Debug(this, "Received a QUIC packet for dcid %s", dcid_hex.c_str());
Debug(this, "Received a QUIC packet for dcid %s", dcid);

BaseObjectPtr<QuicSession> session = FindSession(dcid);

Expand All @@ -489,7 +482,7 @@ void QuicSocket::OnReceive(
// 3. The packet is a stateless reset sent by the peer
// 4. This is a malicious or malformed packet.
if (!session) {
Debug(this, "There is no existing session for dcid %s", dcid_hex.c_str());
Debug(this, "There is no existing session for dcid %s", dcid);
bool is_short_header = IsShortHeader(pversion, pscid, pscidlen);

// Handle possible reception of a stateless reset token...
Expand Down