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

introduce a Transport #3794

Merged
merged 8 commits into from
May 2, 2023
Merged

introduce a Transport #3794

merged 8 commits into from
May 2, 2023

Conversation

marten-seemann
Copy link
Member

@marten-seemann marten-seemann commented Apr 27, 2023

Part of #3727. Fixes #3797.

@marten-seemann marten-seemann changed the base branch from master to listener-structs April 27, 2023 12:59
@marten-seemann marten-seemann force-pushed the new-api branch 12 times, most recently from 5a5d1a6 to d2bd22e Compare April 29, 2023 10:52
@marten-seemann marten-seemann marked this pull request as ready for review April 29, 2023 10:55
@marten-seemann marten-seemann force-pushed the new-api branch 11 times, most recently from 9a842b8 to 98af9d4 Compare May 1, 2023 10:11
@codecov
Copy link

codecov bot commented May 1, 2023

Codecov Report

Merging #3794 (07ad2cb) into master (5400587) will decrease coverage by 0.21%.
The diff coverage is 67.91%.

@@            Coverage Diff             @@
##           master    #3794      +/-   ##
==========================================
- Coverage   84.18%   83.97%   -0.21%     
==========================================
  Files         141      142       +1     
  Lines       14128    14208      +80     
==========================================
+ Hits        11893    11930      +37     
- Misses       1815     1846      +31     
- Partials      420      432      +12     
Impacted Files Coverage Δ
connection.go 74.05% <ø> (+0.01%) ⬆️
interface.go 0.00% <ø> (ø)
logging/multiplex.go 90.00% <ø> (-4.56%) ⬇️
qlog/qlog.go 90.98% <ø> (-0.17%) ⬇️
client.go 57.58% <59.52%> (-20.12%) ⬇️
transport.go 65.66% <65.66%> (ø)
internal/handshake/crypto_setup.go 62.03% <66.67%> (ø)
server.go 77.88% <71.60%> (-1.23%) ⬇️
packet_handler_map.go 84.52% <77.78%> (+16.98%) ⬆️
config.go 100.00% <100.00%> (ø)
... and 3 more

... and 4 files with indirect coverage changes

Copy link
Member

@lucas-clemente lucas-clemente left a comment

Choose a reason for hiding this comment

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

add a GetConfigForClient callback to the Config ### TODO: add tests

one of the commits :)

)

// rawConn is a connection that allow reading of a receivedPacket.
Copy link
Member

Choose a reason for hiding this comment

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

Nit: typo

Copy link
Member Author

Choose a reason for hiding this comment

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

Typo in a removed comment?

transport.go Outdated Show resolved Hide resolved
// The length of the connection ID in bytes.
// It can be 0, or any value between 4 and 18.
// If unset, a 4 byte connection ID will be used.
ConnectionIDLength int
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe omit ConnectionIDLength entirely in favor of ConnectionIDGenerator?

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 consider ConnectionIDGenerator somewhat of a fringe use case. You only need it if you want to use a Connection ID-based load balancer.

if t.ConnectionIDGenerator != nil {
t.connIDLen = t.ConnectionIDGenerator.ConnectionIDLen()
} else {
connIDLen := t.ConnectionIDLength
Copy link
Contributor

Choose a reason for hiding this comment

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

If (Transport).ConnectionIDLength was removed, this branch on t.isSingleUse and isServer can be eliminated.

transport.go Show resolved Hide resolved
@marten-seemann marten-seemann changed the base branch from listener-structs to master May 2, 2023 07:49
@marten-seemann marten-seemann force-pushed the new-api branch 2 times, most recently from c7f9709 to 5fdc5c8 Compare May 2, 2023 09:52
@marten-seemann
Copy link
Member Author

add a GetConfigForClient callback to the Config ### TODO: add tests

one of the commits :)

Added tests. Which caught several bugs :)

@marten-seemann marten-seemann force-pushed the new-api branch 3 times, most recently from 0e6f3c1 to 68ffd08 Compare May 2, 2023 13:44
@marten-seemann marten-seemann merged commit 6b74a9a into master May 2, 2023
30 checks passed
@marten-seemann marten-seemann deleted the new-api branch May 2, 2023 14:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

race condition in key update test
3 participants