Skip to content
This repository has been archived by the owner on Aug 11, 2020. It is now read-only.

quic: multiple refactors #207

Closed
wants to merge 15 commits into from
Closed

Conversation

jasnell
Copy link
Member

@jasnell jasnell commented Dec 2, 2019

Refactoring and cleanup of utility bits and pieces.

  • Use the ngtcp2_path_storage and ngtcp2_path structs directly
  • Refine and clean up QuicError
  • Refactor node_quic_util.cc into an inline header
  • Separate SocketAddress into separate inline header

doc/api/quic.md Outdated Show resolved Hide resolved
doc/api/quic.md Outdated Show resolved Hide resolved
doc/api/quic.md Outdated Show resolved Hide resolved
doc/api/quic.md Outdated Show resolved Hide resolved
doc/api/quic.md Outdated Show resolved Hide resolved
src/node_quic_session.h Outdated Show resolved Hide resolved
src/node_sockaddr-inl.h Outdated Show resolved Hide resolved
src/node_sockaddr-inl.h Outdated Show resolved Hide resolved
src/node_sockaddr.h Show resolved Hide resolved
@@ -98,6 +99,10 @@ class UDPWrapBase {
// Stores the sockaddr for the local socket in `name`.
virtual int GetSockName(sockaddr* name, int* namelen) = 0;

virtual SocketAddress* GetPeerName(SocketAddress* addr = nullptr) = 0;

virtual SocketAddress* GetSockName(SocketAddress* addr = nullptr) = 0;
Copy link
Member

Choose a reason for hiding this comment

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

This should be fine, but in the long run either the SocketAddress* variants or the sockaddr* variants should become non-virtual and implemented in terms of the other, I guess.

Also, could these maybe just return a SocketAddress instead of taking a pointer argument that doubles as the return value?

Copy link
Member Author

Choose a reason for hiding this comment

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

+1 to the first part... will look at that in a future PR.

For the second part, I'll look at that but the current use of this in node_quic_socket.cc updates a member variable of QuicSocket...

udp_->GetSockName(&local_address_);

The return value isn't actually used. I set it up this way just to give some flexibility that we may not actually need in the long run. That is, we could alternatively just drop the return value.

@jasnell jasnell force-pushed the use_ngtcp_path_storage branch from eacc82c to 58a1cd2 Compare December 4, 2019 02:03
Allow for better future reuse of sockaddr related code
by moving SocketAddress into it's own header
Allows monitoring the number of times packets have to be
retransmitted because of loss or delayed ack. An implementation
experiencing a high number of retransmits may need to be torn
down or mitigated in some way. This allows the user code to keep
track. Later, we may want to add a setting that enforces a maximum
number of retransmissions before failing a connection.

Fixes: nodejs#78
@jasnell jasnell force-pushed the use_ngtcp_path_storage branch from 89cad7e to 04dd27d Compare December 4, 2019 02:40

Verified

This commit was signed with the committer’s verified signature.
jasnell James M Snell
@jasnell jasnell force-pushed the use_ngtcp_path_storage branch from 51968d1 to 85c628c Compare December 4, 2019 03:07
jasnell added a commit that referenced this pull request Dec 4, 2019

Verified

This commit was signed with the committer’s verified signature.
jasnell James M Snell
PR-URL: #207
Reviewed-By: #207
jasnell added a commit that referenced this pull request Dec 4, 2019

Verified

This commit was signed with the committer’s verified signature.
jasnell James M Snell
PR-URL: #207
Reviewed-By: #207
jasnell added a commit that referenced this pull request Dec 4, 2019

Verified

This commit was signed with the committer’s verified signature.
jasnell James M Snell
PR-URL: #207
Reviewed-By: #207
jasnell added a commit that referenced this pull request Dec 4, 2019

Verified

This commit was signed with the committer’s verified signature.
jasnell James M Snell
Allow for better future reuse of sockaddr related code
by moving SocketAddress into it's own header

PR-URL: #207
Reviewed-By: #207
jasnell added a commit that referenced this pull request Dec 4, 2019

Verified

This commit was signed with the committer’s verified signature.
jasnell James M Snell
Fixes: #208
PR-URL: #207
Reviewed-By: #207
jasnell added a commit that referenced this pull request Dec 4, 2019

Verified

This commit was signed with the committer’s verified signature.
jasnell James M Snell
PR-URL: #207
Reviewed-By: #207
jasnell added a commit that referenced this pull request Dec 4, 2019

Verified

This commit was signed with the committer’s verified signature.
jasnell James M Snell
Fixes: #208
PR-URL: #207
Reviewed-By: #207
jasnell added a commit that referenced this pull request Dec 4, 2019

Verified

This commit was signed with the committer’s verified signature.
jasnell James M Snell
Fixes: #86
PR-URL: #207
Reviewed-By: #207
jasnell added a commit that referenced this pull request Dec 4, 2019

Verified

This commit was signed with the committer’s verified signature.
jasnell James M Snell
PR-URL: #207
Reviewed-By: #207
jasnell added a commit that referenced this pull request Dec 4, 2019

Verified

This commit was signed with the committer’s verified signature.
jasnell James M Snell
Allows monitoring the number of times packets have to be
retransmitted because of loss or delayed ack. An implementation
experiencing a high number of retransmits may need to be torn
down or mitigated in some way. This allows the user code to keep
track. Later, we may want to add a setting that enforces a maximum
number of retransmissions before failing a connection.

Fixes: #78
PR-URL: #207
Reviewed-By: #207
jasnell added a commit that referenced this pull request Dec 4, 2019

Verified

This commit was signed with the committer’s verified signature.
jasnell James M Snell
Fixes: #58
PR-URL: #207
Reviewed-By: #207
jasnell added a commit that referenced this pull request Dec 4, 2019

Verified

This commit was signed with the committer’s verified signature.
jasnell James M Snell
PR-URL: #207
Reviewed-By: #207
jasnell added a commit that referenced this pull request Dec 4, 2019

Verified

This commit was signed with the committer’s verified signature.
jasnell James M Snell
PR-URL: #207
Reviewed-By: #207
jasnell added a commit that referenced this pull request Dec 4, 2019

Verified

This commit was signed with the committer’s verified signature.
jasnell James M Snell
PR-URL: #207
Reviewed-By: #207
jasnell added a commit that referenced this pull request Dec 4, 2019

Verified

This commit was signed with the committer’s verified signature.
jasnell James M Snell
PR-URL: #207
Reviewed-By: #207
@jasnell
Copy link
Member Author

jasnell commented Dec 4, 2019

Landed

@jasnell jasnell closed this Dec 4, 2019
addaleax pushed a commit that referenced this pull request Dec 11, 2019

Verified

This commit was signed with the committer’s verified signature. The key has expired.
addaleax Anna Henningsen
PR-URL: #207
Reviewed-By: #207
addaleax pushed a commit that referenced this pull request Dec 11, 2019

Verified

This commit was signed with the committer’s verified signature. The key has expired.
addaleax Anna Henningsen
PR-URL: #207
Reviewed-By: #207
addaleax pushed a commit that referenced this pull request Dec 11, 2019

Verified

This commit was signed with the committer’s verified signature. The key has expired.
addaleax Anna Henningsen
PR-URL: #207
Reviewed-By: #207
addaleax pushed a commit that referenced this pull request Dec 11, 2019

Verified

This commit was signed with the committer’s verified signature. The key has expired.
addaleax Anna Henningsen
Allow for better future reuse of sockaddr related code
by moving SocketAddress into it's own header

PR-URL: #207
Reviewed-By: #207
addaleax pushed a commit that referenced this pull request Dec 11, 2019

Verified

This commit was signed with the committer’s verified signature. The key has expired.
addaleax Anna Henningsen
Fixes: #208
PR-URL: #207
Reviewed-By: #207
addaleax pushed a commit that referenced this pull request Dec 11, 2019

Verified

This commit was signed with the committer’s verified signature. The key has expired.
addaleax Anna Henningsen
PR-URL: #207
Reviewed-By: #207
addaleax pushed a commit that referenced this pull request Dec 11, 2019

Verified

This commit was signed with the committer’s verified signature. The key has expired.
addaleax Anna Henningsen
Fixes: #208
PR-URL: #207
Reviewed-By: #207
addaleax pushed a commit that referenced this pull request Dec 11, 2019

Verified

This commit was signed with the committer’s verified signature. The key has expired.
addaleax Anna Henningsen
Fixes: #86
PR-URL: #207
Reviewed-By: #207
addaleax pushed a commit that referenced this pull request Dec 11, 2019

Verified

This commit was signed with the committer’s verified signature. The key has expired.
addaleax Anna Henningsen
PR-URL: #207
Reviewed-By: #207
addaleax pushed a commit that referenced this pull request Dec 11, 2019

Verified

This commit was signed with the committer’s verified signature. The key has expired.
addaleax Anna Henningsen
Allows monitoring the number of times packets have to be
retransmitted because of loss or delayed ack. An implementation
experiencing a high number of retransmits may need to be torn
down or mitigated in some way. This allows the user code to keep
track. Later, we may want to add a setting that enforces a maximum
number of retransmissions before failing a connection.

Fixes: #78
PR-URL: #207
Reviewed-By: #207
addaleax pushed a commit that referenced this pull request Dec 11, 2019

Verified

This commit was signed with the committer’s verified signature. The key has expired.
addaleax Anna Henningsen
Fixes: #58
PR-URL: #207
Reviewed-By: #207
addaleax pushed a commit that referenced this pull request Dec 11, 2019

Verified

This commit was signed with the committer’s verified signature. The key has expired.
addaleax Anna Henningsen
PR-URL: #207
Reviewed-By: #207
addaleax pushed a commit that referenced this pull request Dec 11, 2019

Verified

This commit was signed with the committer’s verified signature. The key has expired.
addaleax Anna Henningsen
PR-URL: #207
Reviewed-By: #207
addaleax pushed a commit that referenced this pull request Dec 11, 2019

Verified

This commit was signed with the committer’s verified signature. The key has expired.
addaleax Anna Henningsen
PR-URL: #207
Reviewed-By: #207
addaleax pushed a commit that referenced this pull request Dec 11, 2019

Verified

This commit was signed with the committer’s verified signature. The key has expired.
addaleax Anna Henningsen
PR-URL: #207
Reviewed-By: #207
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants