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

Disconnect the connection after sending N un-acked packets #234

Merged

Conversation

kstrafe
Copy link
Contributor

@kstrafe kstrafe commented Sep 2, 2019

The connection is disconnected if we have N packets-in-flight
simultaneously. Under normal usage we expect packets to be acked
regularly so that our packets-in-flight size is relatively small.

@kstrafe kstrafe force-pushed the sequenced-pathological-disconnect branch 2 times, most recently from b861dc0 to 35eb8de Compare September 2, 2019 16:16
Copy link
Contributor

@jstnlef jstnlef left a comment

Choose a reason for hiding this comment

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

Nice 👍. The only comment I have is maybe we should make the default something smaller?

@kstrafe
Copy link
Contributor Author

kstrafe commented Sep 3, 2019

@jstnlef What do you propose? This value is basically a "disable" value as it stands.

@kstrafe kstrafe force-pushed the sequenced-pathological-disconnect branch from 35eb8de to d7b1174 Compare September 3, 2019 01:56
@jstnlef
Copy link
Contributor

jstnlef commented Sep 3, 2019

What about 512? Less than 20 seconds at 30 Hz send rate. Let's it be generous without being too soon.

@kstrafe kstrafe force-pushed the sequenced-pathological-disconnect branch from d7b1174 to 365c16b Compare September 3, 2019 04:27
@kstrafe
Copy link
Contributor Author

kstrafe commented Sep 3, 2019

Sounds good

The connection is disconnected if we have N packets-in-flight
simultaneously. Under normal usage we expect packets to be acked
regularly so that our packets-in-flight size is relatively small.
@azriel91
Copy link

azriel91 commented Sep 3, 2019

Had to abort the build (sorry -- the build agent got stuck and had to be restarted). I think you have to push another commit to restart a build.

@codecov
Copy link

codecov bot commented Sep 5, 2019

Codecov Report

Merging #234 into master will decrease coverage by 0.03%.
The diff coverage is 95.34%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #234      +/-   ##
==========================================
- Coverage   97.57%   97.53%   -0.04%     
==========================================
  Files          25       25              
  Lines        2554     2597      +43     
==========================================
+ Hits         2492     2533      +41     
- Misses         62       64       +2
Impacted Files Coverage Δ
src/net/connection.rs 100% <100%> (ø) ⬆️
src/config.rs 100% <100%> (ø) ⬆️
src/net/virtual_connection.rs 96.92% <100%> (+0.01%) ⬆️
src/infrastructure/acknowledgment.rs 99.4% <100%> (+0.01%) ⬆️
src/net/socket.rs 96.14% <93.54%> (-0.13%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1d8917d...365c16b. Read the comment docs.

@kstrafe
Copy link
Contributor Author

kstrafe commented Sep 6, 2019

Codecov is just complaining about panics not being triggered in a test.

@TimonPost TimonPost merged commit a67ddbd into TimonPost:master Sep 7, 2019
jstnlef pushed a commit to jstnlef/laminar that referenced this pull request Oct 12, 2019
…#234)

The connection is disconnected if we have N packets-in-flight
simultaneously. Under normal usage we expect packets to be acked
regularly so that our packets-in-flight size is relatively small.
jstnlef pushed a commit that referenced this pull request Oct 12, 2019
The connection is disconnected if we have N packets-in-flight
simultaneously. Under normal usage we expect packets to be acked
regularly so that our packets-in-flight size is relatively small.
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.

4 participants