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

fix datagram support detection #3511

Merged
merged 3 commits into from Aug 22, 2022
Merged

fix datagram support detection #3511

merged 3 commits into from Aug 22, 2022

Conversation

KevinZonda
Copy link
Contributor

@KevinZonda KevinZonda commented Aug 19, 2022

  • treat zero MaxDatagramFrameSize as unsupported.
  • avoid send corresponding transport parameters when support is disabled
  • report error when calling Send/ReceiveMessage and support is disabled

Fixes #3505.

- treat zero MaxDatagramFrameSize as unsupported.
- avoid send corresponding transport parameters when support is disabled
- report error when calling Send/ReceiveMessage and support is disabled

see #3505
@TNQOYxNU
Copy link

This replace #3507

Copy link
Member

@marten-seemann marten-seemann left a comment

Choose a reason for hiding this comment

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

It seems like the datagramQueue should be initialized when we receive the peer's transport parameters, not on setup, right?

@codecov
Copy link

codecov bot commented Aug 20, 2022

Codecov Report

Base: 85.69% // Head: 85.60% // Decreases project coverage by -0.09% ⚠️

Coverage data is based on head (166d932) compared to base (8c0c481).
Patch coverage: 14.29% of modified lines in pull request are covered.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #3511      +/-   ##
==========================================
- Coverage   85.69%   85.60%   -0.09%     
==========================================
  Files         136      137       +1     
  Lines        9992    10008      +16     
==========================================
+ Hits         8562     8567       +5     
- Misses       1052     1064      +12     
+ Partials      378      377       -1     
Impacted Files Coverage Δ
connection.go 77.64% <14.29%> (-0.02%) ⬇️
closed_conn.go 64.71% <0.00%> (-14.71%) ⬇️
packet_handler_map.go 72.37% <0.00%> (-1.73%) ⬇️
http3/server.go 74.29% <0.00%> (-0.23%) ⬇️
internal/ackhandler/ackhandler.go 0.00% <0.00%> (ø)
internal/handshake/token_generator.go 76.60% <0.00%> (ø)
internal/ackhandler/sent_packet_handler.go 78.01% <0.00%> (ø)
zero_rtt_queue.go 57.14% <0.00%> (ø)
conn_id_generator.go 89.66% <0.00%> (+0.37%) ⬆️
... and 1 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@TNQOYxNU
Copy link

It seems like the datagramQueue should be initialized when we receive the peer's transport parameters, not on setup, right?

I'm neither familiar with code base nor with standard, so this PR doesn't address this problem, only add a nil check to avoid go panic.
To support "unidirectional datagram", it need to be initiated somewhere, initiate it on demand after peer's transport parameter is the best option.
Another option is always initiate it on setup regardless configuration, straightforward but may have some overhead.
We'd better also disable Send/ReceiveMessage on demand, improve doc, even redesign ConnectionState API to avoid confusion--it's two independent unidirectional channel.

@TNQOYxNU
Copy link

Initiate datagramQueue after transport parameter (connection.applyTransportParameters()) not works, seems newPacketPacker need datagramQueue before transport parameter available?
Always initiate it in connection.preSetup() passed all test cases.

@marten-seemann
Copy link
Member

Always initiate it in connection.preSetup() passed all test cases.

Sounds good to me. Then we can send datagrams, even if we don't support receiving datagrams. Can you update the PR?

connection.go Outdated Show resolved Hide resolved
@marten-seemann marten-seemann changed the title fix: datagram support detection fix datagram support detection Aug 22, 2022
@marten-seemann marten-seemann merged commit a901357 into quic-go:master Aug 22, 2022
@KevinZonda KevinZonda deleted the datagram branch August 22, 2022 19:39
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.

ConnectionState.SupportsDatagrams return wrong value
3 participants