Chromium Code Reviews
|
|
Chromium Code Reviews|
Created:
4 years, 11 months ago by joachim Modified:
4 years, 10 months ago Reviewers:
pthatcher1 CC:
webrtc-reviews_webrtc.org, tterriberry_mozilla.com Base URL:
https://chromium.googlesource.com/external/webrtc.git@master Target Ref:
refs/pending/heads/master Project:
webrtc Visibility:
Public. |
DescriptionStay writable after partial socket writes.
This CL fixes an issue where the "writable" flag didn't stay set after
::send or ::sendto only sent a partial buffer.
Also SocketTest::TcpInternal has been updated to use rtc::Buffer instead
of manually allocating data.
BUG=webrtc:4898
Committed: https://crrev.com/f2a2bf4ae448dff45974a7f38f8db005f643f3a3
Cr-Commit-Position: refs/heads/master@{#11480}
Patch Set 1 #Patch Set 2 : Removed unnecessary cast. #Patch Set 3 : Fixed compiler error about signed/unsigned comparison. #
Total comments: 32
Patch Set 4 : Feedback from Peter. #
Total comments: 2
Patch Set 5 : Tentative fix for Mac. #
Total comments: 6
Patch Set 6 : More feedback. #
Created: 4 years, 10 months ago
Messages
Total messages: 33 (11 generated)
jbauch@webrtc.org changed reviewers: + pthatcher@webrtc.org
Ptal, this is another fix for async socket event handling.
On 2016/01/22 23:49:08, joachim wrote: > Ptal, this is another fix for async socket event handling. Ping?
It all looks very good, except that I'd like to make the unit test a bit more readable. I've left some comments with ideas on how. I'll try and be much quicker with the next turn-around on review so you can get this landed. https://codereview.webrtc.org/1616153007/diff/40001/webrtc/base/physicalsocke... File webrtc/base/physicalsocketserver_unittest.cc (right): https://codereview.webrtc.org/1616153007/diff/40001/webrtc/base/physicalsocke... webrtc/base/physicalsocketserver_unittest.cc:76: } Can you flip this around? if (!dispatcher->Initialize()) { delete dispatcher; return nullptr; } return dispatcher; https://codereview.webrtc.org/1616153007/diff/40001/webrtc/base/physicalsocke... webrtc/base/physicalsocketserver_unittest.cc:91: // Maximum size to ::send to a socket. Set to 0 to disable limiting. Can you make "disabled limited" < 0, such as -1. 0 sounds to me like "it can't send", which would be a valid option for a test. https://codereview.webrtc.org/1616153007/diff/40001/webrtc/base/physicalsocke... webrtc/base/physicalsocketserver_unittest.cc:93: int MaximumSendSize() const { return max_send_size_; } Please change MaximumSendSize => MaxSendSize in both places. https://codereview.webrtc.org/1616153007/diff/40001/webrtc/base/physicalsocke... webrtc/base/physicalsocketserver_unittest.cc:104: void PartialWriteStayWritable(const IPAddress& loopback); Can you name this WritableAfterPartialWrite? https://codereview.webrtc.org/1616153007/diff/40001/webrtc/base/physicalsocke... webrtc/base/physicalsocketserver_unittest.cc:264: const int kMaximumSendSize = 128; kMaxSendSize https://codereview.webrtc.org/1616153007/diff/40001/webrtc/base/physicalsocke... webrtc/base/physicalsocketserver_unittest.cc:270: scoped_ptr<char[]> recv_buffer(new char[kDataSize]); It seems like this would benefit from using rtc::Buffer. rtc::Buffer recv_buffer(0, kDataSize); rtc::Buffer send_buffer(0, kDataSize); for (size_t i = 0; i < kDataSize; ++i) { send_buffer.AppendData(static_cast<char>(i % 256)); } Then later: while (recv_buffer.size() < send_buffer.size()) { int unsent_count = send_buffer.size() - sent_size; sender->Send(send_buffer.data() + sent_size, unsent_size); ... char[kDataSize] recved_data; int recved_size receiver->Recv(temp, kDataSize); ... if (recved_data > 0) { recv_buffer.AppendData(recved_data, recved_size); } ... EXPECT_EQ(send_buffer, receive_buffer) https://codereview.webrtc.org/1616153007/diff/40001/webrtc/base/physicalsocke... webrtc/base/physicalsocketserver_unittest.cc:271: size_t send_pos = 0, recv_pos = 0; You can replace all instances of recv_pos with recv_buffer.size(). Can you call send_pos "sent_size" instead? https://codereview.webrtc.org/1616153007/diff/40001/webrtc/base/physicalsocke... webrtc/base/physicalsocketserver_unittest.cc:304: // Send and receive a bunch of data. Can you move the definition of the send and recv buffers to down here just before they are used? I think that would make it more readable. https://codereview.webrtc.org/1616153007/diff/40001/webrtc/base/physicalsocke... webrtc/base/physicalsocketserver_unittest.cc:305: bool send_waiting_for_writability = false; Why not just "writable", with the reversed meaning (writable = true here)? That would seem more clear. Below it would be while (writable && send_pos < kDataSize) { ... } https://codereview.webrtc.org/1616153007/diff/40001/webrtc/base/physicalsocke... webrtc/base/physicalsocketserver_unittest.cc:305: bool send_waiting_for_writability = false; Can you call sender = accepted; receiver = client; That would make the code below more readable. https://codereview.webrtc.org/1616153007/diff/40001/webrtc/base/physicalsocke... webrtc/base/physicalsocketserver_unittest.cc:306: bool send_expect_success = true; Can you call this send_called = false? That would make it more clear how you use it below. https://codereview.webrtc.org/1616153007/diff/40001/webrtc/base/physicalsocke... webrtc/base/physicalsocketserver_unittest.cc:307: bool recv_waiting_for_readability = true; Similarly, "readable = false" here. https://codereview.webrtc.org/1616153007/diff/40001/webrtc/base/physicalsocke... webrtc/base/physicalsocketserver_unittest.cc:308: bool recv_expect_success = false; And this "recv_called = false"? https://codereview.webrtc.org/1616153007/diff/40001/webrtc/base/physicalsocke... webrtc/base/physicalsocketserver_unittest.cc:311: // Send as much as we can if we've been cleared to send. if we've been cleared to send => while we're cleared to send ? https://codereview.webrtc.org/1616153007/diff/40001/webrtc/base/physicalsocke... webrtc/base/physicalsocketserver_unittest.cc:325: data_in_flight += sent; I think you can replace all instances of data_in_flight with (sent_size - recv_buffer.size()) https://codereview.webrtc.org/1616153007/diff/40001/webrtc/base/physicalsocke... webrtc/base/physicalsocketserver_unittest.cc:335: while (data_in_flight > 0) { Or in this case: while (recv_buffer.size() < sent_size)
Description was changed from ========== Stay writable after partial socket writes. This CL fixes an issue where the "writable" flag didn't stay set after ::send or ::sendto only sent a partial buffer. BUG=webrtc:4898 ========== to ========== Stay writable after partial socket writes. This CL fixes an issue where the "writable" flag didn't stay set after ::send or ::sendto only sent a partial buffer. Also SocketTest::TcpInternal has been updated to use rtc::Buffer instead of manually allocating data. BUG=webrtc:4898 ==========
Thanks for your detailed feedback. Please see my replies to your comments for further clarifications on how I updated the test in the latest changes. https://codereview.webrtc.org/1616153007/diff/40001/webrtc/base/physicalsocke... File webrtc/base/physicalsocketserver_unittest.cc (right): https://codereview.webrtc.org/1616153007/diff/40001/webrtc/base/physicalsocke... webrtc/base/physicalsocketserver_unittest.cc:76: } On 2016/01/30 00:03:10, pthatcher1 wrote: > Can you flip this around? > > if (!dispatcher->Initialize()) { > delete dispatcher; > return nullptr; > } > return dispatcher; Done. This was implemented similar to the existing "CreateAsyncSocket" methods, so I changed these as well. https://codereview.webrtc.org/1616153007/diff/40001/webrtc/base/physicalsocke... webrtc/base/physicalsocketserver_unittest.cc:91: // Maximum size to ::send to a socket. Set to 0 to disable limiting. On 2016/01/30 00:03:10, pthatcher1 wrote: > Can you make "disabled limited" < 0, such as -1. 0 sounds to me like "it can't > send", which would be a valid option for a test. Done. https://codereview.webrtc.org/1616153007/diff/40001/webrtc/base/physicalsocke... webrtc/base/physicalsocketserver_unittest.cc:93: int MaximumSendSize() const { return max_send_size_; } On 2016/01/30 00:03:10, pthatcher1 wrote: > Please change MaximumSendSize => MaxSendSize in both places. Done. https://codereview.webrtc.org/1616153007/diff/40001/webrtc/base/physicalsocke... webrtc/base/physicalsocketserver_unittest.cc:104: void PartialWriteStayWritable(const IPAddress& loopback); On 2016/01/30 00:03:10, pthatcher1 wrote: > Can you name this WritableAfterPartialWrite? Done. https://codereview.webrtc.org/1616153007/diff/40001/webrtc/base/physicalsocke... webrtc/base/physicalsocketserver_unittest.cc:264: const int kMaximumSendSize = 128; On 2016/01/30 00:03:10, pthatcher1 wrote: > kMaxSendSize Done. https://codereview.webrtc.org/1616153007/diff/40001/webrtc/base/physicalsocke... webrtc/base/physicalsocketserver_unittest.cc:270: scoped_ptr<char[]> recv_buffer(new char[kDataSize]); On 2016/01/30 00:03:10, pthatcher1 wrote: > It seems like this would benefit from using rtc::Buffer. > > > rtc::Buffer recv_buffer(0, kDataSize); > rtc::Buffer send_buffer(0, kDataSize); > for (size_t i = 0; i < kDataSize; ++i) { > send_buffer.AppendData(static_cast<char>(i % 256)); > } > > > Then later: > > > > while (recv_buffer.size() < send_buffer.size()) { > int unsent_count = send_buffer.size() - sent_size; > sender->Send(send_buffer.data() + sent_size, unsent_size); > ... > > char[kDataSize] recved_data; > int recved_size receiver->Recv(temp, kDataSize); > ... > if (recved_data > 0) { > recv_buffer.AppendData(recved_data, recved_size); > } > > ... > EXPECT_EQ(send_buffer, receive_buffer) Done. As I used "SocketTest::TcpInternal" as base for this test, I updated "TcpInternal" instead with your suggested changes here and below and added parameters to it where it needs to run differently from here. https://codereview.webrtc.org/1616153007/diff/40001/webrtc/base/physicalsocke... webrtc/base/physicalsocketserver_unittest.cc:271: size_t send_pos = 0, recv_pos = 0; On 2016/01/30 00:03:10, pthatcher1 wrote: > You can replace all instances of recv_pos with recv_buffer.size(). > > Can you call send_pos "sent_size" instead? Done. https://codereview.webrtc.org/1616153007/diff/40001/webrtc/base/physicalsocke... webrtc/base/physicalsocketserver_unittest.cc:304: // Send and receive a bunch of data. On 2016/01/30 00:03:10, pthatcher1 wrote: > Can you move the definition of the send and recv buffers to down here just > before they are used? I think that would make it more readable. Done. https://codereview.webrtc.org/1616153007/diff/40001/webrtc/base/physicalsocke... webrtc/base/physicalsocketserver_unittest.cc:305: bool send_waiting_for_writability = false; On 2016/01/30 00:03:10, pthatcher1 wrote: > Can you call > > sender = accepted; > receiver = client; > > That would make the code below more readable. Done. https://codereview.webrtc.org/1616153007/diff/40001/webrtc/base/physicalsocke... webrtc/base/physicalsocketserver_unittest.cc:305: bool send_waiting_for_writability = false; On 2016/01/30 00:03:10, pthatcher1 wrote: > Why not just "writable", with the reversed meaning (writable = true here)? That > would seem more clear. Below it would be while (writable && send_pos < > kDataSize) { ... } Done. https://codereview.webrtc.org/1616153007/diff/40001/webrtc/base/physicalsocke... webrtc/base/physicalsocketserver_unittest.cc:306: bool send_expect_success = true; On 2016/01/30 00:03:10, pthatcher1 wrote: > Can you call this send_called = false? That would make it more clear how you > use it below. Done. https://codereview.webrtc.org/1616153007/diff/40001/webrtc/base/physicalsocke... webrtc/base/physicalsocketserver_unittest.cc:307: bool recv_waiting_for_readability = true; On 2016/01/30 00:03:10, pthatcher1 wrote: > Similarly, "readable = false" here. Done. https://codereview.webrtc.org/1616153007/diff/40001/webrtc/base/physicalsocke... webrtc/base/physicalsocketserver_unittest.cc:308: bool recv_expect_success = false; On 2016/01/30 00:03:10, pthatcher1 wrote: > And this "recv_called = false"? Done. https://codereview.webrtc.org/1616153007/diff/40001/webrtc/base/physicalsocke... webrtc/base/physicalsocketserver_unittest.cc:311: // Send as much as we can if we've been cleared to send. On 2016/01/30 00:03:09, pthatcher1 wrote: > if we've been cleared to send => while we're cleared to send > > ? Done. https://codereview.webrtc.org/1616153007/diff/40001/webrtc/base/physicalsocke... webrtc/base/physicalsocketserver_unittest.cc:325: data_in_flight += sent; On 2016/01/30 00:03:10, pthatcher1 wrote: > I think you can replace all instances of data_in_flight with (sent_size - > recv_buffer.size()) Done. https://codereview.webrtc.org/1616153007/diff/40001/webrtc/base/physicalsocke... webrtc/base/physicalsocketserver_unittest.cc:335: while (data_in_flight > 0) { On 2016/01/30 00:03:10, pthatcher1 wrote: > Or in this case: > > while (recv_buffer.size() < sent_size) Done.
lgtm https://codereview.webrtc.org/1616153007/diff/60001/webrtc/base/socket_unitte... File webrtc/base/socket_unittest.cc (right): https://codereview.webrtc.org/1616153007/diff/60001/webrtc/base/socket_unitte... webrtc/base/socket_unittest.cc:788: EXPECT_EQ(0, memcmp(recv_buffer.data(), send_buffer.data(), data_size)); Doesn't EXPECT_EQ(recv_buffer, send_buffer) work?
The CQ bit was checked by pthatcher@webrtc.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1616153007/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1616153007/60001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: mac_rel on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/mac_rel/builds/12451)
Ptal at the test changes that fix the failures on Mac https://codereview.webrtc.org/1616153007/diff/60001/webrtc/base/socket_unitte... File webrtc/base/socket_unittest.cc (right): https://codereview.webrtc.org/1616153007/diff/60001/webrtc/base/socket_unitte... webrtc/base/socket_unittest.cc:788: EXPECT_EQ(0, memcmp(recv_buffer.data(), send_buffer.data(), data_size)); On 2016/02/01 20:27:13, pthatcher1 wrote: > Doesn't EXPECT_EQ(recv_buffer, send_buffer) work? You're right, buffers implement the "==" operator. Changed.
Can you make just a few more readability tweaks? One for the new line of code, and two more that I noticed along the way :). https://codereview.webrtc.org/1616153007/diff/80001/webrtc/base/socket_unitte... File webrtc/base/socket_unittest.cc (right): https://codereview.webrtc.org/1616153007/diff/80001/webrtc/base/socket_unitte... webrtc/base/socket_unittest.cc:724: int unsent_count = static_cast<int>(send_buffer.size() - sent_size); I just realized that this should be unsent_size, not unsent_count. Can you change it? https://codereview.webrtc.org/1616153007/diff/80001/webrtc/base/socket_unitte... webrtc/base/socket_unittest.cc:736: if (check_max_sent_size < data_size && sent < unsent_count) { Can you rename check_max_sent_size to max_send_size and make -1 mean "unconstrained", and then make this check: if (max_send_size >= 0 && sent < unsent_count) { // If max_send_size is limiting the amount to send per call such that the // sent amount is less than the unsent amount, we simulate that // the socket is no longer writable. writable = false; } https://codereview.webrtc.org/1616153007/diff/80001/webrtc/base/socket_unitte... File webrtc/base/socket_unittest.h (right): https://codereview.webrtc.org/1616153007/diff/80001/webrtc/base/socket_unitte... webrtc/base/socket_unittest.h:70: size_t check_max_sent_size = kDefaultTcpInternalDataSize); Actually, can you make these non-optional arguments and have the caller pass them in?
All changed. https://codereview.webrtc.org/1616153007/diff/80001/webrtc/base/socket_unitte... File webrtc/base/socket_unittest.cc (right): https://codereview.webrtc.org/1616153007/diff/80001/webrtc/base/socket_unitte... webrtc/base/socket_unittest.cc:724: int unsent_count = static_cast<int>(send_buffer.size() - sent_size); On 2016/02/02 18:25:39, pthatcher1 wrote: > I just realized that this should be unsent_size, not unsent_count. Can you > change it? Done. https://codereview.webrtc.org/1616153007/diff/80001/webrtc/base/socket_unitte... webrtc/base/socket_unittest.cc:736: if (check_max_sent_size < data_size && sent < unsent_count) { On 2016/02/02 18:25:39, pthatcher1 wrote: > Can you rename check_max_sent_size to max_send_size and make -1 mean > "unconstrained", and then make this check: > > if (max_send_size >= 0 && sent < unsent_count) { > // If max_send_size is limiting the amount to send per call such that the > // sent amount is less than the unsent amount, we simulate that > // the socket is no longer writable. > writable = false; > } Done. I modified the condition a bit and moved one of the new EXPECT_LE checks. https://codereview.webrtc.org/1616153007/diff/80001/webrtc/base/socket_unitte... File webrtc/base/socket_unittest.h (right): https://codereview.webrtc.org/1616153007/diff/80001/webrtc/base/socket_unitte... webrtc/base/socket_unittest.h:70: size_t check_max_sent_size = kDefaultTcpInternalDataSize); On 2016/02/02 18:25:39, pthatcher1 wrote: > Actually, can you make these non-optional arguments and have the caller pass > them in? Done.
The CQ bit was checked by jbauch@webrtc.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1616153007/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1616153007/100001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_x64_gn_dbg on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/win_x64_gn_dbg/builds/6849)
lgtm
The CQ bit was checked by pthatcher@webrtc.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1616153007/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1616153007/100001
lgtm Land if it's urgent, but please follow up with a unit test.
On 2016/02/03 22:40:03, pthatcher1 wrote: > lgtm > > Land if it's urgent, but please follow up with a unit test. Sorry, that was meant for a different CL. I'll check the commit check box for this one.
On 2016/02/03 22:40:50, pthatcher1 wrote: > On 2016/02/03 22:40:03, pthatcher1 wrote: > > lgtm > > > > Land if it's urgent, but please follow up with a unit test. > > > Sorry, that was meant for a different CL. I'll check the commit check box for > this one. Yeah, figured that :-) I think you already triggered the CQ here.
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_arm64_rel on tryserver.webrtc (JOB_TIMED_OUT, no build URL) linux_baremetal on tryserver.webrtc (JOB_TIMED_OUT, no build URL)
The CQ bit was checked by jbauch@webrtc.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1616153007/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1616153007/100001
Message was sent while issue was closed.
Description was changed from ========== Stay writable after partial socket writes. This CL fixes an issue where the "writable" flag didn't stay set after ::send or ::sendto only sent a partial buffer. Also SocketTest::TcpInternal has been updated to use rtc::Buffer instead of manually allocating data. BUG=webrtc:4898 ========== to ========== Stay writable after partial socket writes. This CL fixes an issue where the "writable" flag didn't stay set after ::send or ::sendto only sent a partial buffer. Also SocketTest::TcpInternal has been updated to use rtc::Buffer instead of manually allocating data. BUG=webrtc:4898 ==========
Message was sent while issue was closed.
Committed patchset #6 (id:100001)
Message was sent while issue was closed.
Description was changed from ========== Stay writable after partial socket writes. This CL fixes an issue where the "writable" flag didn't stay set after ::send or ::sendto only sent a partial buffer. Also SocketTest::TcpInternal has been updated to use rtc::Buffer instead of manually allocating data. BUG=webrtc:4898 ========== to ========== Stay writable after partial socket writes. This CL fixes an issue where the "writable" flag didn't stay set after ::send or ::sendto only sent a partial buffer. Also SocketTest::TcpInternal has been updated to use rtc::Buffer instead of manually allocating data. BUG=webrtc:4898 Committed: https://crrev.com/f2a2bf4ae448dff45974a7f38f8db005f643f3a3 Cr-Commit-Position: refs/heads/master@{#11480} ==========
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/f2a2bf4ae448dff45974a7f38f8db005f643f3a3 Cr-Commit-Position: refs/heads/master@{#11480}
Message was sent while issue was closed.
A revert of this CL (patchset #6 id:100001) has been created in https://codereview.webrtc.org/1670213002/ by ivoc@webrtc.org. The reason for reverting is: This is suspected to cause the race conditions that Linux Tsan v2 started detecting recently. Revert to see if the race conditions go away.. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
