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

Add T3-rtx timer and congestion control #11

Closed
kc5nra opened this issue Jul 24, 2018 · 17 comments
Closed

Add T3-rtx timer and congestion control #11

kc5nra opened this issue Jul 24, 2018 · 17 comments
Assignees
@enobufs

Comments

@kc5nra
Copy link
Contributor

kc5nra commented Jul 24, 2018

https://tools.ietf.org/html/rfc4960#section-6.3.3
@trivigy trivigy self-assigned this Sep 5, 2018
@trivigy

This comment has been minimized.

Sign in to view
@Sean-Der

This comment has been minimized.

Sign in to view
@trivigy

This comment has been minimized.

Sign in to view
@Sean-Der

This comment has been minimized.

Sign in to view
@trivigy

This comment has been minimized.

Sign in to view
@trivigy

This comment has been minimized.

Sign in to view
@trivigy

This comment has been minimized.

Sign in to view
@Sean-Der

This comment has been minimized.

Sign in to view
@trivigy trivigy removed their assignment Sep 15, 2018
@backkem backkem transferred this issue from pion/webrtc Jan 23, 2019
@backkem backkem mentioned this issue Jan 24, 2019
Closed
2 tasks
@enobufs
Copy link
Member

enobufs commented Feb 13, 2019

One consideration:
getPayloadDataToSend() method (will land on master soon) has time.Now() inside it. We should put it outside to make testing easier.

@enobufs enobufs mentioned this issue Feb 13, 2019
Merged
@kc5nra
Copy link
Contributor Author

kc5nra commented Feb 13, 2019

One consideration:
getPayloadDataToSend() method (will land on master soon) has time.Now() inside it. We should put it outside to make testing easier.

As well as time.Since

@backkem backkem mentioned this issue Feb 21, 2019
Closed
@enobufs
Copy link
Member

enobufs commented Feb 28, 2019 •
edited
Loading

I am going to start working on this issue form this weekend. The implementation would somewhat relate to congestion control, #14 and likely #21 would be addressed all together.

During the work of pion/webrtc#334, I ran a test with lossy connections using Network Link Conditioner. I noticed nearly 40% of the time, data channel establishment stalled (at 10% packet loss ratio on both directions) although ICE state becomes connected, which is due to the lack of INIT and COOKIE ECHO retransmissions. This is very crucial for the real world use cases. So, I am thinking about implementing those in this work as well.

Retransmission timers missing:

  • T1-init timer
  • T1-cookie timer
  • T2-shutdown timer (note1)
  • T3-rtx timer
  • T5-shutdown-guard timer (note1)
  • Heartbeat timer (note2)

Note 1: this may be done after the shutdown is fully implemented in a separate issue

Note 2: this may be done after the heartbeat is implemented in a separate issue

enobufs added a commit that referenced this issue Mar 5, 2019
@enobufs
Implement RTO calculation and base timer
88ae212 
Relates to #11
@enobufs enobufs mentioned this issue Mar 5, 2019
Merged
enobufs added a commit that referenced this issue Mar 7, 2019
@enobufs
Improve RTXTimer
34bfbd0 
Added rtoMin/Max cap, timeout failure and more tests.
Fixed some racde codntions
Relates to #11
enobufs added a commit that referenced this issue Mar 11, 2019
@enobufs
Make the timers private
3fc23d6 
Also add timer ID for observers to distinguish timers
Relates to #11
enobufs added a commit to pion/transport that referenced this issue Mar 12, 2019
@enobufs
Added dropNWrites() to Bridge
fbd35b1 
To be used by testing in sctp
Relates to pion/sctp#11
enobufs added a commit that referenced this issue Mar 12, 2019
@enobufs
Implmeneted T1-init and T1-cookie timers
ff02c49 
Also simplified rtxTimer code
Relates to #11
This was referenced Mar 13, 2019
Closed
Closed
@enobufs
Copy link
Member

enobufs commented Mar 15, 2019

I created a separate issue #24 for T1-init/cookie timers as I'd like to make these timers available earlier.

enobufs added a commit that referenced this issue Mar 15, 2019
@enobufs
Implmeneted T1-init and T1-cookie timers
e2773ec 
Also simplified rtxTimer code
Relates to #11
enobufs added a commit to pion/transport that referenced this issue Mar 17, 2019
@enobufs
Added dropNWrites() to Bridge
7f71daf 
To be used by testing in sctp
Relates to pion/sctp#11
enobufs added a commit to pion/transport that referenced this issue Mar 17, 2019
@enobufs
Added dropNWrites() to Bridge
a60953e 
To be used by testing in sctp
Relates to pion/sctp#11
enobufs added a commit that referenced this issue Mar 17, 2019
@enobufs
Implmeneted T1-init and T1-cookie timers
e75c82f 
Also simplified rtxTimer code
Relates to #11
enobufs added a commit that referenced this issue Apr 5, 2019
@enobufs
Modification per review feedback
af5449e 
Use errors.Errorf cosistently within association.go
Resolves #11
enobufs added a commit that referenced this issue Apr 5, 2019
@enobufs
Modification per review feedback
602f899 
Do not capitalize the beginning of the error message
Resolves #11
enobufs added a commit that referenced this issue Apr 5, 2019
@enobufs
Disallow stdlib functions that print to stdio
1f0a37e 
All printing to stdio must be done via pion/logging, all calls to common
printing functions will not cause CI to fail
Resolves #11
@enobufs
Copy link
Member

enobufs commented Apr 6, 2019 •
edited
Loading

I found a bug in handleForwardTSN() sending SACK to every single FowardTSN. The SACK again generates ForwardTSN beyond advancedPeerTSNAckPoint.

According to RFC 3758 Sec 3.6, SACK should only be sent when FowardTSN's newCumulativeTSN is the current peerLastTSN or behind - which was not correctly implemented!

I have locally confirmed that this fixed the "snowballing effect", and my stress test passed.

I will add the change to PR #26 soon. (@Sean-Der)

enobufs added a commit that referenced this issue Apr 6, 2019
@enobufs
Unnecessary sack on receipt of foward TSN
d551d78 
Resolves #11
enobufs added a commit that referenced this issue Apr 6, 2019
@enobufs
Implemeted t3-rtx timer and congestion control
7934f03 
Relates to #11
enobufs added a commit that referenced this issue Apr 6, 2019
@enobufs
Added RTO and fast retransmission test
30a2046 
Relates to #11
enobufs added a commit that referenced this issue Apr 6, 2019
@enobufs
Update per review feedback
5b54952 
Fixed a typo in the comment
Put pendingQueue in a separate file
Made T3 timer not to stop (rtx forever)
Other minor cleanups
Resolves #11
enobufs added a commit that referenced this issue Apr 6, 2019
@enobufs
Fix some golangci-lint errors
43a1635 
Resolves #11
enobufs added a commit that referenced this issue Apr 6, 2019
@enobufs
Fixed golangci-lint errors
fed8556 
Resolves #11
enobufs added a commit that referenced this issue Apr 6, 2019
@enobufs
Fixed unused param error
26b76d9 
Resolves #11
enobufs added a commit that referenced this issue Apr 6, 2019
@enobufs
Implemented bufferedAmount and threshold API
669ec2a 
Resolves #11
enobufs added a commit that referenced this issue Apr 6, 2019
@enobufs
Fix potential race on the callback
d932264 
Resolves #11
enobufs added a commit that referenced this issue Apr 6, 2019
@enobufs
Fixed a bug found by datachannel tests
0fa6e4f 
Optimized the pending queue.
Resolves #11
enobufs added a commit that referenced this issue Apr 6, 2019
@enobufs
Fixed timer starting after association is closed
a6918d0 
Resolves #11
enobufs added a commit that referenced this issue Apr 6, 2019
@enobufs
Fixed race in sendResetRequest
e80632c 
This is unrelated to #11 but revealed in the new test added to webrtc
package.
Relates to #11
enobufs added a commit that referenced this issue Apr 6, 2019
@enobufs
Fixed a bug in cwnd calculation
fb2de16 
Resolves #11
enobufs added a commit that referenced this issue Apr 6, 2019
@enobufs
Initial rwnd from 64KB to 128KB
b7766a6 
Cleaned up comments
Removed one last fmt.Print*
Adjusted Congestion Avoidance test duration
Resolves #11
enobufs added a commit that referenced this issue Apr 6, 2019
@enobufs
Update logging to v0.2.0
9db5272 
Resolves #11
enobufs added a commit that referenced this issue Apr 6, 2019
@enobufs
Take logger factory from upper layer
a3e8aa4 
Resolves #11
enobufs added a commit that referenced this issue Apr 6, 2019
@enobufs
Fix build error with not passing logger factory
1aae7af 
Resolves #11
enobufs added a commit that referenced this issue Apr 6, 2019
@enobufs
Uncomment logs
4cf43b7 
Resolves #11
enobufs added a commit that referenced this issue Apr 6, 2019
@enobufs
Fix pions->pion
d950854 
Resolves #11
enobufs added a commit that referenced this issue Apr 6, 2019
@enobufs
Updated per review feedback
5d81111 
Resolves #11
enobufs added a commit that referenced this issue Apr 6, 2019
@enobufs
Fix typo, added comments
3382e3f 
Resolves #11
enobufs added a commit that referenced this issue Apr 6, 2019
@enobufs
Modification per review feedback
6f9da1a 
Use errors.Errorf cosistently within association.go
Resolves #11
enobufs added a commit that referenced this issue Apr 6, 2019
@enobufs
Modification per review feedback
99e2b30 
Do not capitalize the beginning of the error message
Resolves #11
enobufs added a commit that referenced this issue Apr 6, 2019
@enobufs
Disallow stdlib functions that print to stdio
0773713 
All printing to stdio must be done via pion/logging, all calls to common
printing functions will not cause CI to fail
Resolves #11
enobufs added a commit that referenced this issue Apr 6, 2019
@enobufs
Unnecessary sack on receipt of foward TSN
1738620 
Resolves #11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@enobufs enobufs

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
4 participants
@kc5nra @enobufs @Sean-Der @trivigy