Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2233033002:
Adding test for unordered, fragmented SCTP message delivery. (Closed)
|
Created:
4 years, 4 months ago by Taylor Brandstetter Modified:
4 years, 4 months ago CC:
webrtc-reviews_webrtc.org, tterriberry_mozilla.com, the sun Base URL:
https://chromium.googlesource.com/external/webrtc.git@master Target Ref:
refs/pending/heads/master Project:
webrtc Visibility:
Public. |
DescriptionAdding test for unordered, fragmented SCTP message delivery.
This functionality broke after a recent usrsctp roll. This test would be
useful in catching issues that arise in the future.
BUG=633959
R=honghaiz@webrtc.org, pthatcher@webrtc.org
Committed: https://crrev.com/9b5306c4ef7cb977b8fbf776b9e61295be1f6a08
Cr-Commit-Position: refs/heads/master@{#13823}
Patch Set 1 #
Total comments: 4
Patch Set 2 : Fix the setting of the SCTP MTU. #
Total comments: 2
Patch Set 3 : Remove the MTU-related changes (they'll go in a separate CL) #
Messages
Total messages: 19 (6 generated)
deadbeef@webrtc.org changed reviewers: + honghaiz@webrtc.org, pthatcher@webrtc.org
PTAL. This test caught 3 issues caused by the recent SCTP roll, so it seems valuable to have going into the future.
lgtm https://codereview.webrtc.org/2233033002/diff/1/webrtc/api/peerconnection_uni... File webrtc/api/peerconnection_unittest.cc (right): https://codereview.webrtc.org/2233033002/diff/1/webrtc/api/peerconnection_uni... webrtc/api/peerconnection_unittest.cc:1942: // Otherwise it's not a true "unordered" test. Nit: Should it be "it isn't" or "it is not"? https://codereview.webrtc.org/2233033002/diff/1/webrtc/media/sctp/sctpdataeng... File webrtc/media/sctp/sctpdataengine.cc (right): https://codereview.webrtc.org/2233033002/diff/1/webrtc/media/sctp/sctpdataeng... webrtc/media/sctp/sctpdataengine.cc:38: const int kSctpOverhead = 80; Make it static if it is not used outside of this file. It would be nicer if we can get 80 from the usrsctp header file. Is it the size of a certain struct there? OK to leave it as is if it cannot be found easily.
https://codereview.webrtc.org/2233033002/diff/1/webrtc/api/peerconnection_uni... File webrtc/api/peerconnection_unittest.cc (right): https://codereview.webrtc.org/2233033002/diff/1/webrtc/api/peerconnection_uni... webrtc/api/peerconnection_unittest.cc:1942: // Otherwise it's not a true "unordered" test. On 2016/08/10 19:25:42, honghaiz3 wrote: > Nit: Should it be "it isn't" or "it is not"? They're grammatically equivalent so I think I'll leave it. https://codereview.webrtc.org/2233033002/diff/1/webrtc/media/sctp/sctpdataeng... File webrtc/media/sctp/sctpdataengine.cc (right): https://codereview.webrtc.org/2233033002/diff/1/webrtc/media/sctp/sctpdataeng... webrtc/media/sctp/sctpdataengine.cc:38: const int kSctpOverhead = 80; On 2016/08/10 19:25:42, honghaiz3 wrote: > Make it static if it is not used outside of this file. > > It would be nicer if we can get 80 from the usrsctp header file. > Is it the size of a certain struct there? > OK to leave it as is if it cannot be found easily. I went to investigate where the 80 is coming from, but I found that the real issue is that the MTU we set is being completely ignored. We just didn't notice because 1200+80 = 1280 is the default MTU. So the mysterious 80 additional bytes were just the difference between the ignored value we set and the one that's actually used. I fixed this by changing where the setsockopt is called and adding the necessary address parameter. Maybe this should go in a separate CL, though, since it's really independent from adding this test. Also... Maybe it's wrong that we were trying to disable MTU discovery in the first place. https://tools.ietf.org/html/draft-ietf-rtcweb-data-channel-13 says it MUST be supported. Peter, do you know anything about this?
I agree that we should have separate CLs for adding the test and for the fix. https://codereview.webrtc.org/2233033002/diff/20001/webrtc/media/sctp/sctpdat... File webrtc/media/sctp/sctpdataengine.cc (right): https://codereview.webrtc.org/2233033002/diff/20001/webrtc/media/sctp/sctpdat... webrtc/media/sctp/sctpdataengine.cc:554: // We can only do this after usrsctp_connect or it has no effect. Does it mean that previously MTU-discovery was not actually disabled but with this change, it will be disabled?
https://codereview.webrtc.org/2233033002/diff/20001/webrtc/media/sctp/sctpdat... File webrtc/media/sctp/sctpdataengine.cc (right): https://codereview.webrtc.org/2233033002/diff/20001/webrtc/media/sctp/sctpdat... webrtc/media/sctp/sctpdataengine.cc:554: // We can only do this after usrsctp_connect or it has no effect. On 2016/08/11 16:54:28, honghaiz3 wrote: > Does it mean that previously MTU-discovery was not actually disabled but with > this change, it will be disabled? Correct. But I'm thinking it may have been a mistake to disable it in the first place (see previous comment). We can discuss that on the new CL: https://codereview.webrtc.org/2237073002/
lgtm
Please update the CL description.
Description was changed from ========== Adding test for unordered, fragmented SCTP message delivery. This functionality broke after a recent usrsctp roll. This test would be useful in catching issues that arise in the future. Also fixed the MTU calculation. If we know that SCTP adds 80 bytes of overhead and we assume a safe MTU of 1280 bytes, then we should really ask for 1280 - 80 - DTLS/TURN/TCP/IP overhead. BUG=633959 ========== to ========== Adding test for unordered, fragmented SCTP message delivery. This functionality broke after a recent usrsctp roll. This test would be useful in catching issues that arise in the future. BUG=633959 ==========
On 2016/08/11 17:46:58, honghaiz3 wrote: > Please update the CL description. Fixed, thanks for noticing that.
lgtm
The CQ bit was checked by deadbeef@webrtc.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_arm64_rel on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/android_arm64_rel/build...)
Description was changed from ========== Adding test for unordered, fragmented SCTP message delivery. This functionality broke after a recent usrsctp roll. This test would be useful in catching issues that arise in the future. BUG=633959 ========== to ========== Adding test for unordered, fragmented SCTP message delivery. This functionality broke after a recent usrsctp roll. This test would be useful in catching issues that arise in the future. BUG=633959 R=honghaiz@webrtc.org, pthatcher@webrtc.org Committed: https://chromium.googlesource.com/external/webrtc/+/9b5306c4ef7cb977b8fbf776b... ==========
Message was sent while issue was closed.
Description was changed from ========== Adding test for unordered, fragmented SCTP message delivery. This functionality broke after a recent usrsctp roll. This test would be useful in catching issues that arise in the future. BUG=633959 R=honghaiz@webrtc.org, pthatcher@webrtc.org Committed: https://chromium.googlesource.com/external/webrtc/+/9b5306c4ef7cb977b8fbf776b... ========== to ========== Adding test for unordered, fragmented SCTP message delivery. This functionality broke after a recent usrsctp roll. This test would be useful in catching issues that arise in the future. BUG=633959 R=honghaiz@webrtc.org, pthatcher@webrtc.org Committed: https://crrev.com/9b5306c4ef7cb977b8fbf776b9e61295be1f6a08 Cr-Commit-Position: refs/heads/master@{#13823} ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001) manually as 9b5306c4ef7cb977b8fbf776b9e61295be1f6a08 (presubmit successful).
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/9b5306c4ef7cb977b8fbf776b9e61295be1f6a08 Cr-Commit-Position: refs/heads/master@{#13823} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
