Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2593063003:
Add Socket::ReadIfReady() (Closed)
DescriptionAdd Socket::ReadIfReady()
This CL adds a new read method to socket class. The new read method
allows caller to not retain an IOBuffer if desired.
The new functionality is hidden behind a field trial flag:
SocketReadIfReady.
The design doc is at:
https://docs.google.com/document/d/1DsVcQ4-jsaIhu_OvIid-UHMUQfFqWYfmThMnQoZ1csc/edit#
BUG=690915
Review-Url: https://codereview.chromium.org/2593063003
Cr-Commit-Position: refs/heads/master@{#455177}
Committed: https://chromium.googlesource.com/chromium/src/+/321a96f376e57651a1ad987f5585c0f07128e63a
Patch Set 1 : fixed tests #Patch Set 2 : Fix tests for real #
Total comments: 42
Patch Set 3 : rebased #Patch Set 4 : Addressed most of comments #Patch Set 5 : Attempt at FakeBlockingStreamSocket #Patch Set 6 : fix asan #Patch Set 7 : Self #
Total comments: 38
Patch Set 8 : Address comments #Patch Set 9 : self #Patch Set 10 : self #Patch Set 11 : Fix perf tests (removed invalid CHECKs) #
Total comments: 20
Patch Set 12 : pure rebase before addressing comments #Patch Set 13 : Address David's comments #Patch Set 14 : self review. remove unused include #
Total comments: 90
Patch Set 15 : Address bnc comments #
Total comments: 2
Patch Set 16 : Address Bence comment #Patch Set 17 : Rebased #
Messages
Total messages: 86 (62 generated)
Description was changed from ========== TEMP DO NOT REVIEW BUG= ========== to ========== TEMP DO NOT REVIEW BUG=524258 ==========
The CQ bit was checked by xunjieli@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)
Patchset #1 (id:1) has been deleted
Patchset #1 (id:20001) has been deleted
Patchset #1 (id:40001) has been deleted
Patchset #1 (id:60001) has been deleted
Patchset #1 (id:80001) has been deleted
Patchset #1 (id:100001) has been deleted
Patchset #1 (id:120001) has been deleted
Patchset #1 (id:140001) has been deleted
Patchset #1 (id:160001) has been deleted
Description was changed from ========== TEMP DO NOT REVIEW BUG=524258 ========== to ========== Add Socket::ReadIfReady() This CL adds a new read method to socket class. The new read method allows caller to not retain an IOBuffer if desired. The design doc is at: https://docs.google.com/document/d/1DsVcQ4-jsaIhu_OvIid-UHMUQfFqWYfmThMnQoZ1c... The new functionality is hidden behind a field trial flag. BUG=524258 ==========
The CQ bit was checked by xunjieli@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by xunjieli@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
Description was changed from ========== Add Socket::ReadIfReady() This CL adds a new read method to socket class. The new read method allows caller to not retain an IOBuffer if desired. The design doc is at: https://docs.google.com/document/d/1DsVcQ4-jsaIhu_OvIid-UHMUQfFqWYfmThMnQoZ1c... The new functionality is hidden behind a field trial flag. BUG=524258 ========== to ========== Add Socket::ReadIfReady() This CL adds a new read method to socket class. The new read method allows caller to not retain an IOBuffer if desired. The new functionality is hidden behind a field trial flag. The design doc is at: https://docs.google.com/document/d/1DsVcQ4-jsaIhu_OvIid-UHMUQfFqWYfmThMnQoZ1c... BUG=524258 ==========
Patchset #3 (id:220001) has been deleted
Patchset #1 (id:180001) has been deleted
Patchset #1 (id:200001) has been deleted
The CQ bit was checked by xunjieli@chromium.org to run a CQ dry run
Patchset #2 (id:260001) has been deleted
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
xunjieli@chromium.org changed reviewers: + davidben@chromium.org
David: This is not ready, but I'd like to get some early feedback in case I am going down on the wrong path. I am sending it out before the weekend so I don't forget about it. Please don't feel obliged to review it til next week. Have a great weekend! :)
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
David, a friendly ping!
Did a review pass. Overall this looks good. Comments are mostly around whether we could make sockets that implement both Read() and ReadIfReady() a little simpler. https://codereview.chromium.org/2593063003/diff/280001/net/base/net_error_list.h File net/base/net_error_list.h (right): https://codereview.chromium.org/2593063003/diff/280001/net/base/net_error_lis... net/base/net_error_list.h:389: // Socket "read if ready" support is not implemented. Unreasonably nitpicky nitpick: I'd maybe say ReadIfReady just so readers have something to grep for. https://codereview.chromium.org/2593063003/diff/280001/net/socket/socket.cc File net/socket/socket.cc (right): https://codereview.chromium.org/2593063003/diff/280001/net/socket/socket.cc#n... net/socket/socket.cc:11: const char Socket::kReadIfReadyTrialName[] = "SocketReadIfReady"; I think base::Feature is the preferred way to do these sorts of toggles nowadays. (Mostly it saves having to mess with strings.) Then you'd add a static Socket::IsReadIfReadyEnabled() or something like that. Would make calling code simpler too. Right now all the callers have to replicate the string thing. https://codereview.chromium.org/2593063003/diff/280001/net/socket/socket_bio_... File net/socket/socket_bio_adapter_unittest.cc (right): https://codereview.chromium.org/2593063003/diff/280001/net/socket/socket_bio_... net/socket/socket_bio_adapter_unittest.cc:32: enum ReadIfReadySupport { Thanks for being thorough with testing this! https://codereview.chromium.org/2593063003/diff/280001/net/socket/socket_bio_... net/socket/socket_bio_adapter_unittest.cc:43: public testing::WithParamInterface<ReadIfReadySupport>, I think you can replace this and the line above with testing::TestWithParam<ReadIfReadySupport>. (Or do we prefer this setup in Chromium? I could be out-of-date on style.) https://codereview.chromium.org/2593063003/diff/280001/net/socket/socket_bio_... net/socket/socket_bio_adapter_unittest.cc:150: DCHECK_GE(OK, rv); Nit: EXPECT. Ditto below. https://codereview.chromium.org/2593063003/diff/280001/net/socket/socket_posi... File net/socket/socket_posix.cc (right): https://codereview.chromium.org/2593063003/diff/280001/net/socket/socket_posi... net/socket/socket_posix.cc:286: } Nit: It might be a little clearer to make lines 253 through 270 be ReadIfReady and then do: int SocketPosix::Read(...) { int rv = ReadIfReady(buf, buf_len, callback); if (rv == ERR_IO_PENDING) { // Save |buf| so ReadCompleted does not call the callback until // driving the read to completion. read_buf_ = buf; read_buf_len_ = callback; } return rv; } It's the same thing, but ReadIfReady feels like the more low-level operation. ----- An alternative to this and the comment below, though it involves stashing two callbacks, is to implement Read() entirely on top of ReadIfReady(). This could be done for basically all sockets. So you'd store two operations worth of state: ReadIfReady(): read_if_ready_callback_ Read(): read_buf_ read_buf_len_ read_callback_ Everything would act as if Read() and its state doesn't exist. Then you implement Read() basically universally as: int Read(buf, buf_len, callback) { int rv = ReadIfReady(buf, buf_len, &RetryRead); if (rv != ERR_IO_PENDING) return rv; read_buf_ = buf; read_buf_len_ = buf_len; read_callback_ = callback; } void RetryRead(int rv) { if (rv == OK) { rv = ReadIfReady(read_buf_, read_buf_len_, &RetryRead); if (rv == ERR_IO_PENDING) return; } read_buf_ = nullptr; read_buf_len_ = 0; ResetAndReturn(&read_callback, rv); } Though if you do this for SocketPosix, you'll bounce on {Start,Stop}WatchingFileDescriptor which is a little unfortunate. Maybe this would be better for the higher-level ones? ----- What do you think? https://codereview.chromium.org/2593063003/diff/280001/net/socket/socket_posi... net/socket/socket_posix.cc:480: base::ResetAndReturn(&read_callback_).Run(rv); You could also write this: int rv = OK; // If |read_buf_| is set, the caller used Read() rather than // ReadIfReady(). Continue attempting to read and only return // when completed. if (read_buf_) { rv = DoRead(read_buf_.get(), read_buf_len_); if (rv == ERR_IO_PENDING) return; } bool ok = .... ... base::ResetAndReturn(.... } This avoids duplicating the chunk at the bottom https://codereview.chromium.org/2593063003/diff/280001/net/socket/socket_test... File net/socket/socket_test_util.cc (right): https://codereview.chromium.org/2593063003/diff/280001/net/socket/socket_test... net/socket/socket_test_util.cc:1: // Copyright (c) 2012 The Chromium Authors. All rights reserved. mmenke may be a better reviewer for this file. I'm not very familiar with what it's doing. Is it possible to pattern it after the socket_posix.cc suggestion? (Supposing, of course, you agree with it.) https://codereview.chromium.org/2593063003/diff/280001/net/socket/socket_test... net/socket/socket_test_util.cc:905: DCHECK(!pending_read_buf_); DCHECK_EQ(NONE, pending_read_type_); to match? https://codereview.chromium.org/2593063003/diff/280001/net/socket/socket_test... net/socket/socket_test_util.cc:1099: bool use_read_if_ready) { This parameter doesn't seem to be used. Did you mean to pass it into CompleteRead? https://codereview.chromium.org/2593063003/diff/280001/net/socket/ssl_client_... File net/socket/ssl_client_socket_impl.cc (right): https://codereview.chromium.org/2593063003/diff/280001/net/socket/ssl_client_... net/socket/ssl_client_socket_impl.cc:886: user_read_buf_len_ = 0; Hrm. Rather than let Read() set up some state and undo it, what if you implemented Read() on top of ReadIfReady(), like the socket_posix.cc suggestion? DoPayloadRead is basically socket_posix.cc's DoRead, just it doesn't take arguments. You could make it take those arguments and have ReadIfReady just pass them in. RetryAllOperations would check if user_read_buf_ is set and, if so, call DoPayloadRead on it rather than calling the read callback immediately. https://codereview.chromium.org/2593063003/diff/280001/net/socket/ssl_client_... net/socket/ssl_client_socket_impl.cc:919: // If ReadIfReady() is called instead of Read(). This logic might be better in RetryAllOperations(). There's actually a third case that Read[IfReady] might be blocked, OnPrivateKeyComplete. This does mean we'll wake up ReadIfReady in cases where we may not need to, but that's hopefully fine? If we need to later, we can track the exact operation each side is blocking on. https://codereview.chromium.org/2593063003/diff/280001/net/socket/ssl_client_... net/socket/ssl_client_socket_impl.cc:922: return; This return means we won't wake up other operations if we have a Read() and Write() flowing at the same time. https://codereview.chromium.org/2593063003/diff/280001/net/socket/ssl_client_... File net/socket/ssl_client_socket_unittest.cc (right): https://codereview.chromium.org/2593063003/diff/280001/net/socket/ssl_client_... net/socket/ssl_client_socket_unittest.cc:242: const CompletionCallback& callback) { Similar comment. I think you can structure this like: - Read calls ReadIfReady and, if ERR_IO_PENDING, stashes user_read_buf_. - ReadCommon inlines into ReadIfReady and doesn't set user_read_buf_. - No need for boolean. - DoRead can just call transport_->Read; the tests only care about the external behavior of ReadBufferingStreamSocket and leaving it alone is simpler. - DoReadComplete checks user_read_buf_ and does the ReadIfReady (just return OK) or Read (memcpy and return capacity) thing accordingly. https://codereview.chromium.org/2593063003/diff/280001/net/socket/ssl_client_... net/socket/ssl_client_socket_unittest.cc:653: base::Unretained(this))); Hrm. This one's a little interesting. If ReadIfReady returns synchronously while should_block_read_ is true, I think things will get confused here. Because you've actually written to the buffer and taken data out of the socket, but you're not returning it yet which means the caller thinks it can throw that data away. (This socket blocks read *results* rather than read *calls*. I think there was some awkward False Start test synchronizing reason why we needed that. Though arguably we care less about those tests now that the code is actually in BoringSSL.) I'm actually not sure how FakeBlockingStreamSocket could implement ReadIfReady + BlockReadResult + WaitForReadResult. Also ReplaceReadResult. Without those two functions, I think BlockRead and BlockReadResult are the same though. https://codereview.chromium.org/2593063003/diff/280001/net/socket/ssl_client_... net/socket/ssl_client_socket_unittest.cc:981: return rv; Since this is test code, do we want to have this check? Otherwise if SSLClientSocket just didn't bother implementing ReadIfReady, I think it'd pass all the tests. https://codereview.chromium.org/2593063003/diff/280001/net/socket/ssl_client_... net/socket/ssl_client_socket_unittest.cc:990: TestCompletionCallback* callback) { Since you pass in and wait on callback in the same function, I think you don't need to pass it in and can instead just create your own one in this function. https://codereview.chromium.org/2593063003/diff/280001/net/socket/ssl_client_... net/socket/ssl_client_socket_unittest.cc:1000: if (rv != ERR_READ_IF_READY_NOT_IMPLEMENTED) Ditto re ERR_READ_IF_READY_NOT_IMPLEMENTED. https://codereview.chromium.org/2593063003/diff/280001/net/socket/ssl_client_... net/socket/ssl_client_socket_unittest.cc:3778: #if defined(OS_POSIX) Not a TEST_P? https://codereview.chromium.org/2593063003/diff/280001/net/socket/tcp_socket_... File net/socket/tcp_socket_posix.cc (right): https://codereview.chromium.org/2593063003/diff/280001/net/socket/tcp_socket_... net/socket/tcp_socket_posix.cc:621: if (tcp_fastopen_write_attempted_ && !tcp_fastopen_connected_) { Should this code also run in the ReadIfReadyCompleted case? https://codereview.chromium.org/2593063003/diff/280001/net/spdy/spdy_session.cc File net/spdy/spdy_session.cc (right): https://codereview.chromium.org/2593063003/diff/280001/net/spdy/spdy_session.... net/spdy/spdy_session.cc:1452: READ_STATE_DO_READ)); Hah. That's much simpler than I expected!
The CQ bit was checked by xunjieli@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Patchset #6 (id:360001) has been deleted
Patchset #5 (id:340001) has been deleted
Thanks for the thorough review! I have addressed comments and uploaded a new patch. I like your Read()/ReadIfReady() split better. The only downside I can see is that we will add a WeakPtrFactory to SocketPosix because we are wrapping the user read callback. I also attempted to fix FakeBlockingStreamSocket by using a temp buffer to hold the blocked synchronous ReadIfReady() result. PTAL. https://codereview.chromium.org/2593063003/diff/280001/net/base/net_error_list.h File net/base/net_error_list.h (right): https://codereview.chromium.org/2593063003/diff/280001/net/base/net_error_lis... net/base/net_error_list.h:389: // Socket "read if ready" support is not implemented. On 2017/02/01 22:25:57, davidben wrote: > Unreasonably nitpicky nitpick: I'd maybe say ReadIfReady just so readers have > something to grep for. Done. https://codereview.chromium.org/2593063003/diff/280001/net/socket/socket.cc File net/socket/socket.cc (right): https://codereview.chromium.org/2593063003/diff/280001/net/socket/socket.cc#n... net/socket/socket.cc:11: const char Socket::kReadIfReadyTrialName[] = "SocketReadIfReady"; On 2017/02/01 22:25:57, davidben wrote: > I think base::Feature is the preferred way to do these sorts of toggles > nowadays. (Mostly it saves having to mess with strings.) Then you'd add a static > Socket::IsReadIfReadyEnabled() or something like that. > > Would make calling code simpler too. Right now all the callers have to replicate > the string thing. Done. Neat! https://codereview.chromium.org/2593063003/diff/280001/net/socket/socket_bio_... File net/socket/socket_bio_adapter_unittest.cc (right): https://codereview.chromium.org/2593063003/diff/280001/net/socket/socket_bio_... net/socket/socket_bio_adapter_unittest.cc:32: enum ReadIfReadySupport { On 2017/02/01 22:25:57, davidben wrote: > Thanks for being thorough with testing this! :) https://codereview.chromium.org/2593063003/diff/280001/net/socket/socket_bio_... net/socket/socket_bio_adapter_unittest.cc:43: public testing::WithParamInterface<ReadIfReadySupport>, On 2017/02/01 22:25:57, davidben wrote: > I think you can replace this and the line above with > testing::TestWithParam<ReadIfReadySupport>. (Or do we prefer this setup in > Chromium? I could be out-of-date on style.) Done. Thanks! I thought there was one class for this purpose but couldn't find it. https://codereview.chromium.org/2593063003/diff/280001/net/socket/socket_bio_... net/socket/socket_bio_adapter_unittest.cc:150: DCHECK_GE(OK, rv); On 2017/02/01 22:25:57, davidben wrote: > Nit: EXPECT. Ditto below. Done. https://codereview.chromium.org/2593063003/diff/280001/net/socket/socket_posi... File net/socket/socket_posix.cc (right): https://codereview.chromium.org/2593063003/diff/280001/net/socket/socket_posi... net/socket/socket_posix.cc:286: } On 2017/02/01 22:25:57, davidben wrote: > Nit: It might be a little clearer to make lines 253 through 270 be ReadIfReady > and then do: > > int SocketPosix::Read(...) { > int rv = ReadIfReady(buf, buf_len, callback); > if (rv == ERR_IO_PENDING) { > // Save |buf| so ReadCompleted does not call the callback until > // driving the read to completion. > read_buf_ = buf; > read_buf_len_ = callback; > } > return rv; > } > > It's the same thing, but ReadIfReady feels like the more low-level operation. > > ----- > > An alternative to this and the comment below, though it involves stashing two > callbacks, is to implement Read() entirely on top of ReadIfReady(). This could > be done for basically all sockets. So you'd store two operations worth of state: > > ReadIfReady(): > read_if_ready_callback_ > > Read(): > read_buf_ > read_buf_len_ > read_callback_ > > Everything would act as if Read() and its state doesn't exist. Then you > implement Read() basically universally as: > > int Read(buf, buf_len, callback) { > int rv = ReadIfReady(buf, buf_len, &RetryRead); > if (rv != ERR_IO_PENDING) > return rv; > > read_buf_ = buf; > read_buf_len_ = buf_len; > read_callback_ = callback; > } > > void RetryRead(int rv) { > if (rv == OK) { > rv = ReadIfReady(read_buf_, read_buf_len_, &RetryRead); > if (rv == ERR_IO_PENDING) > return; > } > > read_buf_ = nullptr; > read_buf_len_ = 0; > ResetAndReturn(&read_callback, rv); > } > > Though if you do this for SocketPosix, you'll bounce on > {Start,Stop}WatchingFileDescriptor which is a little unfortunate. Maybe this > would be better for the higher-level ones? > > ----- > > What do you think? Done. I like your approach better. It's easier to understand. Thanks for the suggestion. But we wouldn't bounce on {Start, Stop}WatchingFileDescriptor more often for the regular Read() than before, right? Read() -> ReadIfReady() -> StartWatching -> ReadCompleted() -> StopWatching -> RetryRead -> DoRead(). There's only one pair of {Start, Stop} for a single Read(), same as before, right? https://codereview.chromium.org/2593063003/diff/280001/net/socket/socket_posi... net/socket/socket_posix.cc:480: base::ResetAndReturn(&read_callback_).Run(rv); On 2017/02/01 22:25:57, davidben wrote: > You could also write this: > > int rv = OK; > // If |read_buf_| is set, the caller used Read() rather than > // ReadIfReady(). Continue attempting to read and only return > // when completed. > if (read_buf_) { > rv = DoRead(read_buf_.get(), read_buf_len_); > if (rv == ERR_IO_PENDING) > return; > } > > bool ok = .... > ... > base::ResetAndReturn(.... > } > > This avoids duplicating the chunk at the bottom Acknowledged. Thanks. This is no longer applicable after applying the the previous suggestion. https://codereview.chromium.org/2593063003/diff/280001/net/socket/socket_test... File net/socket/socket_test_util.cc (right): https://codereview.chromium.org/2593063003/diff/280001/net/socket/socket_test... net/socket/socket_test_util.cc:1: // Copyright (c) 2012 The Chromium Authors. All rights reserved. On 2017/02/01 22:25:57, davidben wrote: > mmenke may be a better reviewer for this file. I'm not very familiar with what > it's doing. Is it possible to pattern it after the socket_posix.cc suggestion? > (Supposing, of course, you agree with it.) Done. https://codereview.chromium.org/2593063003/diff/280001/net/socket/socket_test... net/socket/socket_test_util.cc:905: DCHECK(!pending_read_buf_); On 2017/02/01 22:25:57, davidben wrote: > DCHECK_EQ(NONE, pending_read_type_); to match? Acknowledged. After applying the suggested Read()/ReadIfReady() split, this is no longer applicable. https://codereview.chromium.org/2593063003/diff/280001/net/socket/socket_test... net/socket/socket_test_util.cc:1099: bool use_read_if_ready) { On 2017/02/01 22:25:57, davidben wrote: > This parameter doesn't seem to be used. Did you mean to pass it into > CompleteRead? Acknowledged. After applying the suggested Read()/ReadIfReady() split, this is no longer applicable. https://codereview.chromium.org/2593063003/diff/280001/net/socket/ssl_client_... File net/socket/ssl_client_socket_impl.cc (right): https://codereview.chromium.org/2593063003/diff/280001/net/socket/ssl_client_... net/socket/ssl_client_socket_impl.cc:886: user_read_buf_len_ = 0; On 2017/02/01 22:25:57, davidben wrote: > Hrm. Rather than let Read() set up some state and undo it, what if you > implemented Read() on top of ReadIfReady(), like the socket_posix.cc suggestion? > > DoPayloadRead is basically socket_posix.cc's DoRead, just it doesn't take > arguments. You could make it take those arguments and have ReadIfReady just pass > them in. RetryAllOperations would check if user_read_buf_ is set and, if so, > call DoPayloadRead on it rather than calling the read callback immediately. Done. https://codereview.chromium.org/2593063003/diff/280001/net/socket/ssl_client_... net/socket/ssl_client_socket_impl.cc:919: // If ReadIfReady() is called instead of Read(). On 2017/02/01 22:25:57, davidben wrote: > This logic might be better in RetryAllOperations(). There's actually a third > case that Read[IfReady] might be blocked, OnPrivateKeyComplete. > > This does mean we'll wake up ReadIfReady in cases where we may not need to, but > that's hopefully fine? If we need to later, we can track the exact operation > each side is blocking on. Done. Ah, there's a third case! Should have done a grep. https://codereview.chromium.org/2593063003/diff/280001/net/socket/ssl_client_... net/socket/ssl_client_socket_impl.cc:922: return; On 2017/02/01 22:25:57, davidben wrote: > This return means we won't wake up other operations if we have a Read() and > Write() flowing at the same time. Acknowledged. https://codereview.chromium.org/2593063003/diff/280001/net/socket/ssl_client_... File net/socket/ssl_client_socket_unittest.cc (right): https://codereview.chromium.org/2593063003/diff/280001/net/socket/ssl_client_... net/socket/ssl_client_socket_unittest.cc:242: const CompletionCallback& callback) { On 2017/02/01 22:25:58, davidben wrote: > Similar comment. I think you can structure this like: > > - Read calls ReadIfReady and, if ERR_IO_PENDING, stashes user_read_buf_. > - ReadCommon inlines into ReadIfReady and doesn't set user_read_buf_. > - No need for boolean. > - DoRead can just call transport_->Read; the tests only care about the external > behavior of ReadBufferingStreamSocket and leaving it alone is simpler. > - DoReadComplete checks user_read_buf_ and does the ReadIfReady (just return OK) > or Read (memcpy and return capacity) thing accordingly. Done. https://codereview.chromium.org/2593063003/diff/280001/net/socket/ssl_client_... net/socket/ssl_client_socket_unittest.cc:653: base::Unretained(this))); On 2017/02/01 22:25:57, davidben wrote: > Hrm. This one's a little interesting. If ReadIfReady returns synchronously while > should_block_read_ is true, I think things will get confused here. Because > you've actually written to the buffer and taken data out of the socket, but > you're not returning it yet which means the caller thinks it can throw that data > away. (This socket blocks read *results* rather than read *calls*. I think there > was some awkward False Start test synchronizing reason why we needed that. > Though arguably we care less about those tests now that the code is actually in > BoringSSL.) > > I'm actually not sure how FakeBlockingStreamSocket could implement ReadIfReady + > BlockReadResult + WaitForReadResult. Also ReplaceReadResult. Without those two > functions, I think BlockRead and BlockReadResult are the same though. Ah, sorry I missed that. Good catch! How about we create a temp buffer to hold the result? That way, if caller throws away |buf|, we can still unblock/replace correctly? I uploaded a new patch. https://codereview.chromium.org/2593063003/diff/280001/net/socket/ssl_client_... net/socket/ssl_client_socket_unittest.cc:981: return rv; On 2017/02/01 22:25:58, davidben wrote: > Since this is test code, do we want to have this check? Otherwise if > SSLClientSocket just didn't bother implementing ReadIfReady, I think it'd pass > all the tests. Done. https://codereview.chromium.org/2593063003/diff/280001/net/socket/ssl_client_... net/socket/ssl_client_socket_unittest.cc:990: TestCompletionCallback* callback) { On 2017/02/01 22:25:58, davidben wrote: > Since you pass in and wait on callback in the same function, I think you don't > need to pass it in and can instead just create your own one in this function. Done. https://codereview.chromium.org/2593063003/diff/280001/net/socket/ssl_client_... net/socket/ssl_client_socket_unittest.cc:1000: if (rv != ERR_READ_IF_READY_NOT_IMPLEMENTED) On 2017/02/01 22:25:58, davidben wrote: > Ditto re ERR_READ_IF_READY_NOT_IMPLEMENTED. Done. https://codereview.chromium.org/2593063003/diff/280001/net/socket/ssl_client_... net/socket/ssl_client_socket_unittest.cc:3778: #if defined(OS_POSIX) On 2017/02/01 22:25:58, davidben wrote: > Not a TEST_P? Done. https://codereview.chromium.org/2593063003/diff/280001/net/socket/tcp_socket_... File net/socket/tcp_socket_posix.cc (right): https://codereview.chromium.org/2593063003/diff/280001/net/socket/tcp_socket_... net/socket/tcp_socket_posix.cc:621: if (tcp_fastopen_write_attempted_ && !tcp_fastopen_connected_) { On 2017/02/01 22:25:58, davidben wrote: > Should this code also run in the ReadIfReadyCompleted case? Done. https://codereview.chromium.org/2593063003/diff/280001/net/spdy/spdy_session.cc File net/spdy/spdy_session.cc (right): https://codereview.chromium.org/2593063003/diff/280001/net/spdy/spdy_session.... net/spdy/spdy_session.cc:1452: READ_STATE_DO_READ)); On 2017/02/01 22:25:58, davidben wrote: > Hah. That's much simpler than I expected! Acknowledged :)
The CQ bit was checked by xunjieli@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
David, a friendly ping. Thanks!
Description was changed from ========== Add Socket::ReadIfReady() This CL adds a new read method to socket class. The new read method allows caller to not retain an IOBuffer if desired. The new functionality is hidden behind a field trial flag. The design doc is at: https://docs.google.com/document/d/1DsVcQ4-jsaIhu_OvIid-UHMUQfFqWYfmThMnQoZ1c... BUG=524258 ========== to ========== Add Socket::ReadIfReady() This CL adds a new read method to socket class. The new read method allows caller to not retain an IOBuffer if desired. The new functionality is hidden behind a field trial flag. The design doc is at: https://docs.google.com/document/d/1DsVcQ4-jsaIhu_OvIid-UHMUQfFqWYfmThMnQoZ1c... BUG=690915 ==========
The CQ bit was checked by xunjieli@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== Add Socket::ReadIfReady() This CL adds a new read method to socket class. The new read method allows caller to not retain an IOBuffer if desired. The new functionality is hidden behind a field trial flag. The design doc is at: https://docs.google.com/document/d/1DsVcQ4-jsaIhu_OvIid-UHMUQfFqWYfmThMnQoZ1c... BUG=690915 ========== to ========== Add Socket::ReadIfReady() This CL adds a new read method to socket class. The new read method allows caller to not retain an IOBuffer if desired. The new functionality is hidden behind a field trial flag: SocketReadIfReady. The design doc is at: https://docs.google.com/document/d/1DsVcQ4-jsaIhu_OvIid-UHMUQfFqWYfmThMnQoZ1c... BUG=690915 ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
I feel like it says something odd about the world that adding ReadIfReady support to test classes is much harder than adding it to our actual ones... https://codereview.chromium.org/2593063003/diff/420001/net/socket/socket.h File net/socket/socket.h (right): https://codereview.chromium.org/2593063003/diff/420001/net/socket/socket.h#ne... net/socket/socket.h:48: // invoked with the error code. Hrm. One thing I noticed that's actually quite subtle about this API: If ReadIfReady *synchronously* returns 0, that means EOF. If it *asynchronously* returns 0, that means "please call me again". https://codereview.chromium.org/2593063003/diff/420001/net/socket/socket_bio_... File net/socket/socket_bio_adapter.cc (right): https://codereview.chromium.org/2593063003/diff/420001/net/socket/socket_bio_... net/socket/socket_bio_adapter.cc:14: #include "base/metrics/field_trial.h" No longer used. https://codereview.chromium.org/2593063003/diff/420001/net/socket/socket_bio_... net/socket/socket_bio_adapter.cc:93: if (base::FeatureList::IsEnabled(Socket::kReadIfReadyExperiment)) { #include "base/feature_list.h" https://codereview.chromium.org/2593063003/diff/420001/net/socket/socket_posi... File net/socket/socket_posix.cc (right): https://codereview.chromium.org/2593063003/diff/420001/net/socket/socket_posi... net/socket/socket_posix.cc:255: weak_factory_.GetWeakPtr())); Do you need a WeakPtr here? Since the callback won't get called after the object is destroyed, I would think base::Unretained(this) would work fine. https://codereview.chromium.org/2593063003/diff/420001/net/socket/socket_test... File net/socket/socket_test_util.cc (right): https://codereview.chromium.org/2593063003/diff/420001/net/socket/socket_test... net/socket/socket_test_util.cc:909: base::Bind(&MockTCPClientSocket::RetryRead, weak_factory_.GetWeakPtr())); Ditto re whether you actually need the WeakPtr. https://codereview.chromium.org/2593063003/diff/420001/net/socket/socket_test... net/socket/socket_test_util.cc:1122: // to complete the async IO manually later (via OnReadComplete). Hrm. Does this feature work for ReadIfReady? (I don't really understand how these classes work.) https://codereview.chromium.org/2593063003/diff/420001/net/socket/socket_test... net/socket/socket_test_util.cc:1139: result = std::min(buf_len, read_data_.data_len - read_offset_); If you've got some data in here and read_data_.mode is ASYNC, it seems like this will consume data when it's not supposed to. https://codereview.chromium.org/2593063003/diff/420001/net/socket/socket_test... net/socket/socket_test_util.cc:1157: weak_factory_.GetWeakPtr(), result)); Does RunCallbackAsync(callback, result) not work? https://codereview.chromium.org/2593063003/diff/420001/net/socket/ssl_client_... File net/socket/ssl_client_socket_impl.cc (right): https://codereview.chromium.org/2593063003/diff/420001/net/socket/ssl_client_... net/socket/ssl_client_socket_impl.cc:1557: rv_read = rv; Hrm. I'm not sure this is quite right. Suppose we get OnWriteReady(ERR_FAILED). Read() may or may not have been blocked on the underlying write channel. You only want to forward the error if it was blocked. All operations are safe to pump whether they're ready or not, so rather than keep track of what everything is blocked on, this code just retries everything for simplicity. Instead, I believe if you remove the return value and just do: rv_read = OK; It will work? Even if we woke up because the transport Read was stuck on ERR_CONNECTION_CLOSED, the caller will ReadIfReady() again and then we'll synchronously get back the ERR_CONNECTION_CLOSED back out. (SocketBIOAdapter stashes the error and replays it, to satisfy the "it is always safe to retry a blocked op" constraint.) https://codereview.chromium.org/2593063003/diff/420001/net/socket/ssl_client_... File net/socket/ssl_client_socket_unittest.cc (right): https://codereview.chromium.org/2593063003/diff/420001/net/socket/ssl_client_... net/socket/ssl_client_socket_unittest.cc:9: #include <algorithm> Nit: Swap this and the blank line below. https://codereview.chromium.org/2593063003/diff/420001/net/socket/ssl_client_... net/socket/ssl_client_socket_unittest.cc:464: std::string blocked_sync_read_if_ready_data_; Hah! Writing into a buffer like that hadn't occured to me. Good idea. https://codereview.chromium.org/2593063003/diff/420001/net/socket/ssl_client_... net/socket/ssl_client_socket_unittest.cc:529: if (has_blocked_sync_read_if_ready_) { I think this doesn't quite work if should_block_read_ is true. Consider this scenario: BlockReadResult() ReadIfReady(10) => get 10 bytes, but should_block_read_ so it goes to a buffer. UnblockReadResult() => ReadIfReady completes with OK. ReadIfReady(5) => okay, we consume 5 bytes from the stashed buffer. BlockReadResult() ReadIfReady(5) => we have bytes in the stashed buffer, but need to resolve it later. But this is also a test socket and I think we'd never do that? We can probably just stick a bunch of CHECKs and simplify this. See comment below. https://codereview.chromium.org/2593063003/diff/420001/net/socket/ssl_client_... net/socket/ssl_client_socket_unittest.cc:571: pending_read_buf_ = new StringIOBuffer(blocked_sync_read_if_ready_data_); Do you need both the std::string and the StringIOBuffer? This looks a little funny. StringIOBuffer makes a copy of its input, so if you modify things later with ReplaceReadResult, it doesn't actually notice. I like the idea to redirect it to another buffer though. What if we implemented ReadIfReady using Read? CompletionCallback read_if_ready_callback_; string read_if_ready_buf_; int FBSS::ReadIfReady(buf, len, callback) { if (!read_if_ready_buf_.empty()) { // This is possible if the caller calls ReadIfReady with a smaller // buffer size than previously or if BlockReadResult is called // after ReadIfReady reports there is data available but before the // data is drained. CHECK(!should_block_read_); CHECK(len >= read_if_ready_buf_.size()); int rv = read_if_read_buf_.size(); memcpy(buf->data(), read_if_read_buf_.data(), rv); read_if_ready_buf_.clear(); return rv; } buf_copy = new IOBuffer(len); int rv = Read(read_if_ready_buf_, len, Bind(CompleteReadIfReady, buf_copy)); if (rv > 0) memcpy(buf->data(), buf_copy->data(), rv); if (rv == ERR_IO_PENDING) read_if_ready_callback_ = callback; return rv; } void FBSS::CompleteReadIfReady(IOBuffer* buf, int rv) { DCHECK(read_if_ready_buf_.empty()); DCHECK(!should_block_read_); if (rv > 0) { read_if_ready_buf_ = string(buf->data(), buf->data() + rv); rv = OK; } ResetAndReturn(read_if_ready_callback_, rv); } (If one of those CHECKs crashes, you might need to make this a little more complicated. Notably the read_if_ready_buf_ + should_block_read_ case will probably require UnblockReadResult to be aware of read_if_ready_callback_ whereas right now should_block_read_ is almost entirely hidden from ReadIfReady.) https://codereview.chromium.org/2593063003/diff/420001/net/socket/ssl_client_... net/socket/ssl_client_socket_unittest.cc:1014: return rv; It's legal for ReadIfReady to signal OK before it's actually ready to complete. (SSLClientSocketImpl will do this since we don't keep perfect track of what is blocking on what.) I suspect this needs to be a loop to avoid flakiness. Something like: int rv; do { int rv = socket->ReadIfReady(...); if (rv != ERR_IO_PENDING) break; rv = callback.GetResult(rv); } while (rv == OK); ----- Edit: Actually, I see you sometimes split things up above. You might need something like: int Read(StreamSocket* socket, IOBuffer* buf, int buf_len, TestCompletionCallback* callback) { if (GetParam()) return socket->ReadIfReady(buf, buf_len, callback->callback()); return socket->Read(buf, buf_len, callback->callback()); } int WaitForReadCompletion(StreamSocket* socket, IOBuffer* buf, int buf_len, TestCompletionCallback* callback, int rv) { if (!GetParam()) return callback->GetResult(rv); while (rv == ERR_IO_PENDING) { rv = callback->GetResult(rv); if (rv != OK) return rv; rv = socket->ReadIfReady(buf, buf_len, callback.callback()) } return rv; } int ReadAndWaitForCompletion(StreamSocket* socket, IOBuffer* buf, int buf_len) { TestCompletionCallback callback; int rv = Read(socket, buf, buf_len, &callback); return WaitForReadCompletion(socket, buf, buf_len, &callback, rv); } https://codereview.chromium.org/2593063003/diff/420001/net/socket/ssl_client_... net/socket/ssl_client_socket_unittest.cc:1672: rv = Read(sock_.get(), buf.get(), 4096, callback.callback()); Given the issue with ReadIfReady always needing to be driven to completion in a loop, perhaps we need a helper function for completing a Read|ReadIfReady result. (See amended comment above.) https://codereview.chromium.org/2593063003/diff/420001/net/socket/ssl_client_... net/socket/ssl_client_socket_unittest.cc:1752: EXPECT_THAT(rv, IsError(ERR_CONNECTION_RESET)); You'll probably need WaitForReadResult here, once you change SSLClientSocketImpl to require an extra loop to surface errors. Probably in a bunch of other places too. https://codereview.chromium.org/2593063003/diff/420001/net/socket/ssl_client_... net/socket/ssl_client_socket_unittest.cc:3797: buffer_released = true; Probably wants a comment for what's happening here. (That you only implemented it for POSIX and not Windows I assume.) https://codereview.chromium.org/2593063003/diff/420001/net/spdy/spdy_session.cc File net/spdy/spdy_session.cc (right): https://codereview.chromium.org/2593063003/diff/420001/net/spdy/spdy_session.... net/spdy/spdy_session.cc:17: #include "base/metrics/field_trial.h" No longer used. https://codereview.chromium.org/2593063003/diff/420001/net/spdy/spdy_session.... net/spdy/spdy_session.cc:1788: if (base::FeatureList::IsEnabled(Socket::kReadIfReadyExperiment)) { #include "base/feature_list.h"
> I feel like it says something odd about the world that adding ReadIfReady > support to test classes is much harder than adding it to our actual ones... I agree. The test classes are way harder to get right. Thanks for the suggestions. PTAL. https://codereview.chromium.org/2593063003/diff/420001/net/socket/socket.h File net/socket/socket.h (right): https://codereview.chromium.org/2593063003/diff/420001/net/socket/socket.h#ne... net/socket/socket.h:48: // invoked with the error code. On 2017/02/10 23:33:48, davidben wrote: > Hrm. One thing I noticed that's actually quite subtle about this API: > > If ReadIfReady *synchronously* returns 0, that means EOF. > > If it *asynchronously* returns 0, that means "please call me again". Acknowledged. Yes, that's right. I have edited the doc. Hopefully it's clearer? https://codereview.chromium.org/2593063003/diff/420001/net/socket/socket_bio_... File net/socket/socket_bio_adapter.cc (right): https://codereview.chromium.org/2593063003/diff/420001/net/socket/socket_bio_... net/socket/socket_bio_adapter.cc:14: #include "base/metrics/field_trial.h" On 2017/02/10 23:33:48, davidben wrote: > No longer used. Done. https://codereview.chromium.org/2593063003/diff/420001/net/socket/socket_bio_... net/socket/socket_bio_adapter.cc:93: if (base::FeatureList::IsEnabled(Socket::kReadIfReadyExperiment)) { On 2017/02/10 23:33:48, davidben wrote: > #include "base/feature_list.h" Done. https://codereview.chromium.org/2593063003/diff/420001/net/socket/socket_posi... File net/socket/socket_posix.cc (right): https://codereview.chromium.org/2593063003/diff/420001/net/socket/socket_posi... net/socket/socket_posix.cc:255: weak_factory_.GetWeakPtr())); On 2017/02/10 23:33:48, davidben wrote: > Do you need a WeakPtr here? Since the callback won't get called after the object > is destroyed, I would think base::Unretained(this) would work fine. Done. You are right. I confused this one with the one where you stash callback onto a task runner. https://codereview.chromium.org/2593063003/diff/420001/net/socket/socket_test... File net/socket/socket_test_util.cc (right): https://codereview.chromium.org/2593063003/diff/420001/net/socket/socket_test... net/socket/socket_test_util.cc:909: base::Bind(&MockTCPClientSocket::RetryRead, weak_factory_.GetWeakPtr())); On 2017/02/10 23:33:48, davidben wrote: > Ditto re whether you actually need the WeakPtr. Done. https://codereview.chromium.org/2593063003/diff/420001/net/socket/socket_test... net/socket/socket_test_util.cc:1122: // to complete the async IO manually later (via OnReadComplete). On 2017/02/10 23:33:48, davidben wrote: > Hrm. Does this feature work for ReadIfReady? (I don't really understand how > these classes work.) I believe it does, but I might be missing something. MockTCPClientSocket::OnReadComplete() will be called by SocketDataProvider. OnReadComplete() calls RetryRead() or the real user read callback. https://codereview.chromium.org/2593063003/diff/420001/net/socket/socket_test... net/socket/socket_test_util.cc:1139: result = std::min(buf_len, read_data_.data_len - read_offset_); On 2017/02/10 23:33:48, davidben wrote: > If you've got some data in here and read_data_.mode is ASYNC, it seems like this > will consume data when it's not supposed to. Done. Ah, good catch! https://codereview.chromium.org/2593063003/diff/420001/net/socket/socket_test... net/socket/socket_test_util.cc:1157: weak_factory_.GetWeakPtr(), result)); On 2017/02/10 23:33:48, davidben wrote: > Does RunCallbackAsync(callback, result) not work? Done. https://codereview.chromium.org/2593063003/diff/420001/net/socket/ssl_client_... File net/socket/ssl_client_socket_impl.cc (right): https://codereview.chromium.org/2593063003/diff/420001/net/socket/ssl_client_... net/socket/ssl_client_socket_impl.cc:1557: rv_read = rv; On 2017/02/10 23:33:48, davidben wrote: > Hrm. I'm not sure this is quite right. Suppose we get OnWriteReady(ERR_FAILED). > Read() may or may not have been blocked on the underlying write channel. You > only want to forward the error if it was blocked. All operations are safe to > pump whether they're ready or not, so rather than keep track of what everything > is blocked on, this code just retries everything for simplicity. > > Instead, I believe if you remove the return value and just do: > > rv_read = OK; > > It will work? Even if we woke up because the transport Read was stuck on > ERR_CONNECTION_CLOSED, the caller will ReadIfReady() again and then we'll > synchronously get back the ERR_CONNECTION_CLOSED back out. (SocketBIOAdapter > stashes the error and replays it, to satisfy the "it is always safe to retry a > blocked op" constraint.) Done. https://codereview.chromium.org/2593063003/diff/420001/net/socket/ssl_client_... File net/socket/ssl_client_socket_unittest.cc (right): https://codereview.chromium.org/2593063003/diff/420001/net/socket/ssl_client_... net/socket/ssl_client_socket_unittest.cc:9: #include <algorithm> On 2017/02/10 23:33:48, davidben wrote: > Nit: Swap this and the blank line below. Done. https://codereview.chromium.org/2593063003/diff/420001/net/socket/ssl_client_... net/socket/ssl_client_socket_unittest.cc:464: std::string blocked_sync_read_if_ready_data_; On 2017/02/10 23:33:48, davidben wrote: > Hah! Writing into a buffer like that hadn't occured to me. Good idea. Acknowledged. https://codereview.chromium.org/2593063003/diff/420001/net/socket/ssl_client_... net/socket/ssl_client_socket_unittest.cc:529: if (has_blocked_sync_read_if_ready_) { On 2017/02/10 23:33:48, davidben wrote: > I think this doesn't quite work if should_block_read_ is true. Consider this > scenario: > > BlockReadResult() > ReadIfReady(10) => get 10 bytes, but should_block_read_ so it goes to a > buffer. > UnblockReadResult() => ReadIfReady completes with OK. > ReadIfReady(5) => okay, we consume 5 bytes from the stashed buffer. > BlockReadResult() > ReadIfReady(5) => we have bytes in the stashed buffer, but need to resolve it > later. > > But this is also a test socket and I think we'd never do that? We can probably > just stick a bunch of CHECKs and simplify this. See comment below. Acknowledged. https://codereview.chromium.org/2593063003/diff/420001/net/socket/ssl_client_... net/socket/ssl_client_socket_unittest.cc:571: pending_read_buf_ = new StringIOBuffer(blocked_sync_read_if_ready_data_); On 2017/02/10 23:33:48, davidben wrote: > Do you need both the std::string and the StringIOBuffer? This looks a little > funny. StringIOBuffer makes a copy of its input, so if you modify things later > with ReplaceReadResult, it doesn't actually notice. > > I like the idea to redirect it to another buffer though. What if we implemented > ReadIfReady using Read? > > CompletionCallback read_if_ready_callback_; > string read_if_ready_buf_; > > int FBSS::ReadIfReady(buf, len, callback) { > if (!read_if_ready_buf_.empty()) { > // This is possible if the caller calls ReadIfReady with a smaller > // buffer size than previously or if BlockReadResult is called > // after ReadIfReady reports there is data available but before the > // data is drained. > CHECK(!should_block_read_); > CHECK(len >= read_if_ready_buf_.size()); > > int rv = read_if_read_buf_.size(); > memcpy(buf->data(), read_if_read_buf_.data(), rv); > read_if_ready_buf_.clear(); > return rv; > } > > buf_copy = new IOBuffer(len); > int rv = Read(read_if_ready_buf_, len, Bind(CompleteReadIfReady, buf_copy)); > if (rv > 0) > memcpy(buf->data(), buf_copy->data(), rv); > if (rv == ERR_IO_PENDING) > read_if_ready_callback_ = callback; > return rv; > } > > void FBSS::CompleteReadIfReady(IOBuffer* buf, int rv) { > DCHECK(read_if_ready_buf_.empty()); > DCHECK(!should_block_read_); > > if (rv > 0) { > read_if_ready_buf_ = string(buf->data(), buf->data() + rv); > rv = OK; > } > > ResetAndReturn(read_if_ready_callback_, rv); > } > > (If one of those CHECKs crashes, you might need to make this a little more > complicated. Notably the read_if_ready_buf_ + should_block_read_ case will > probably require UnblockReadResult to be aware of read_if_ready_callback_ > whereas right now should_block_read_ is almost entirely hidden from > ReadIfReady.) Done. Neat! I somehow though StringIOBuffer doesn't own the underlying buffer or do a string copy https://codereview.chromium.org/2593063003/diff/420001/net/socket/ssl_client_... net/socket/ssl_client_socket_unittest.cc:1014: return rv; On 2017/02/10 23:33:48, davidben wrote: > It's legal for ReadIfReady to signal OK before it's actually ready to complete. > (SSLClientSocketImpl will do this since we don't keep perfect track of what is > blocking on what.) I suspect this needs to be a loop to avoid flakiness. > Something like: > > int rv; > do { > int rv = socket->ReadIfReady(...); > if (rv != ERR_IO_PENDING) > break; > rv = callback.GetResult(rv); > } while (rv == OK); > > ----- > > Edit: Actually, I see you sometimes split things up above. You might need > something like: > > int Read(StreamSocket* socket, IOBuffer* buf, int buf_len, > TestCompletionCallback* callback) { > if (GetParam()) > return socket->ReadIfReady(buf, buf_len, callback->callback()); > return socket->Read(buf, buf_len, callback->callback()); > } > > int WaitForReadCompletion(StreamSocket* socket, IOBuffer* buf, int buf_len, > TestCompletionCallback* callback, int rv) { > if (!GetParam()) > return callback->GetResult(rv); > > while (rv == ERR_IO_PENDING) { > rv = callback->GetResult(rv); > if (rv != OK) > return rv; > rv = socket->ReadIfReady(buf, buf_len, callback.callback()) > } > > return rv; > } > > int ReadAndWaitForCompletion(StreamSocket* socket, > IOBuffer* buf, > int buf_len) { > TestCompletionCallback callback; > int rv = Read(socket, buf, buf_len, &callback); > return WaitForReadCompletion(socket, buf, buf_len, &callback, rv); > } Done. https://codereview.chromium.org/2593063003/diff/420001/net/socket/ssl_client_... net/socket/ssl_client_socket_unittest.cc:1672: rv = Read(sock_.get(), buf.get(), 4096, callback.callback()); On 2017/02/10 23:33:48, davidben wrote: > Given the issue with ReadIfReady always needing to be driven to completion in a > loop, perhaps we need a helper function for completing a Read|ReadIfReady > result. > > (See amended comment above.) Done. https://codereview.chromium.org/2593063003/diff/420001/net/socket/ssl_client_... net/socket/ssl_client_socket_unittest.cc:1752: EXPECT_THAT(rv, IsError(ERR_CONNECTION_RESET)); On 2017/02/10 23:33:48, davidben wrote: > You'll probably need WaitForReadResult here, once you change SSLClientSocketImpl > to require an extra loop to surface errors. Probably in a bunch of other places > too. Actually this particularly one deletes the socket during OnComplete(), so we can't continue with ReadIfReady(). I update the test case accordingly. PTAL. https://codereview.chromium.org/2593063003/diff/420001/net/socket/ssl_client_... net/socket/ssl_client_socket_unittest.cc:3797: buffer_released = true; On 2017/02/10 23:33:48, davidben wrote: > Probably wants a comment for what's happening here. (That you only implemented > it for POSIX and not Windows I assume.) Done. https://codereview.chromium.org/2593063003/diff/420001/net/spdy/spdy_session.cc File net/spdy/spdy_session.cc (right): https://codereview.chromium.org/2593063003/diff/420001/net/spdy/spdy_session.... net/spdy/spdy_session.cc:17: #include "base/metrics/field_trial.h" On 2017/02/10 23:33:49, davidben wrote: > No longer used. Done. https://codereview.chromium.org/2593063003/diff/420001/net/spdy/spdy_session.... net/spdy/spdy_session.cc:1788: if (base::FeatureList::IsEnabled(Socket::kReadIfReadyExperiment)) { On 2017/02/10 23:33:49, davidben wrote: > #include "base/feature_list.h" Done.
And I also fixed the perf tests. It was due to invalid DCHECKs that I put in RetryAllOperations(). You can see the change in diffs between #10 and #11. Thanks!
xunjieli@chromium.org changed reviewers: + bnc@chromium.org
+ bnc@: PTAL. Thanks!
Minor comments, but otherwise I think this is good. lgtm! https://codereview.chromium.org/2593063003/diff/500001/net/socket/socket_test... File net/socket/socket_test_util.cc (right): https://codereview.chromium.org/2593063003/diff/500001/net/socket/socket_test... net/socket/socket_test_util.cc:924: return ReadIfReadyHelper(buf, buf_len, callback); Nit: Perhaps ReadIfReadyHelper => ReadIfReadyImpl? It looks like you're going the strategy of implementing Read over ReadIfReady, except the helper is needed to bypass the enable_read_if_ready_ check. https://codereview.chromium.org/2593063003/diff/500001/net/socket/socket_test... net/socket/socket_test_util.cc:1057: RunCallback(callback, read_data_.result > 0 ? OK : read_data_.result); Nit: This is probably a little tidier: RunCallback(base::ResetAndReturn(&pending_read_if_ready_callback_), read_data_.result > 0 ? OK : read_data_.result); https://codereview.chromium.org/2593063003/diff/500001/net/socket/socket_test... net/socket/socket_test_util.cc:1100: RunCallback(callback, rv); Nit: RunCallback(base::ResetAndReturn(&pending_read_callback_), rv); https://codereview.chromium.org/2593063003/diff/500001/net/socket/socket_test... net/socket/socket_test_util.cc:1135: was_used_to_convey_data_ = true; I think you want to move this to after the ASYNC check since we haven't gotten data out quite yet. https://codereview.chromium.org/2593063003/diff/500001/net/socket/ssl_client_... File net/socket/ssl_client_socket_impl.cc (right): https://codereview.chromium.org/2593063003/diff/500001/net/socket/ssl_client_... net/socket/ssl_client_socket_impl.cc:1537: void SSLClientSocketImpl::RetryAllOperations(int rv) { rv is no longer used. https://codereview.chromium.org/2593063003/diff/500001/net/socket/ssl_client_... File net/socket/ssl_client_socket_unittest.cc (right): https://codereview.chromium.org/2593063003/diff/500001/net/socket/ssl_client_... net/socket/ssl_client_socket_unittest.cc:9: #include <algorithm> Nit: Swap this and the blank line below. https://codereview.chromium.org/2593063003/diff/500001/net/socket/ssl_client_... net/socket/ssl_client_socket_unittest.cc:535: // drained. The comment should probably be clearer that we're explicitly not supporting these cases for simplicity. https://codereview.chromium.org/2593063003/diff/500001/net/socket/ssl_client_... net/socket/ssl_client_socket_unittest.cc:537: CHECK_GE(len, (int)read_if_ready_buf_.size()); Nit: static_cast<int>(read_if_ready_buf_.size()) https://codereview.chromium.org/2593063003/diff/500001/net/socket/ssl_client_... net/socket/ssl_client_socket_unittest.cc:602: DCHECK_NE(-1, pending_read_buf_len_); 601 and 602 are already checked a few lines above. Why does 600 need to move after the if? https://codereview.chromium.org/2593063003/diff/500001/net/socket/ssl_client_... net/socket/ssl_client_socket_unittest.cc:1488: EXPECT_THAT(rv, IsError(ERR_CONNECTION_RESET)); ReadAndWaitForCompletion?
The CQ bit was checked by xunjieli@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Thanks! I will wait for Bence to get a chance to review. I will clean up my other WIP CL and upload it for review. https://codereview.chromium.org/2593063003/diff/500001/net/socket/socket_test... File net/socket/socket_test_util.cc (right): https://codereview.chromium.org/2593063003/diff/500001/net/socket/socket_test... net/socket/socket_test_util.cc:924: return ReadIfReadyHelper(buf, buf_len, callback); On 2017/03/02 20:47:02, davidben wrote: > Nit: Perhaps ReadIfReadyHelper => ReadIfReadyImpl? It looks like you're going > the strategy of implementing Read over ReadIfReady, except the helper is needed > to bypass the enable_read_if_ready_ check. Done. https://codereview.chromium.org/2593063003/diff/500001/net/socket/socket_test... net/socket/socket_test_util.cc:1057: RunCallback(callback, read_data_.result > 0 ? OK : read_data_.result); On 2017/03/02 20:47:02, davidben wrote: > Nit: This is probably a little tidier: > > RunCallback(base::ResetAndReturn(&pending_read_if_ready_callback_), > read_data_.result > 0 ? OK : read_data_.result); Done. https://codereview.chromium.org/2593063003/diff/500001/net/socket/socket_test... net/socket/socket_test_util.cc:1100: RunCallback(callback, rv); On 2017/03/02 20:47:02, davidben wrote: > Nit: RunCallback(base::ResetAndReturn(&pending_read_callback_), rv); Done. https://codereview.chromium.org/2593063003/diff/500001/net/socket/socket_test... net/socket/socket_test_util.cc:1135: was_used_to_convey_data_ = true; On 2017/03/02 20:47:02, davidben wrote: > I think you want to move this to after the ASYNC check since we haven't gotten > data out quite yet. Done. Good catch. You are right! https://codereview.chromium.org/2593063003/diff/500001/net/socket/ssl_client_... File net/socket/ssl_client_socket_impl.cc (right): https://codereview.chromium.org/2593063003/diff/500001/net/socket/ssl_client_... net/socket/ssl_client_socket_impl.cc:1537: void SSLClientSocketImpl::RetryAllOperations(int rv) { On 2017/03/02 20:47:02, davidben wrote: > rv is no longer used. Done. https://codereview.chromium.org/2593063003/diff/500001/net/socket/ssl_client_... File net/socket/ssl_client_socket_unittest.cc (right): https://codereview.chromium.org/2593063003/diff/500001/net/socket/ssl_client_... net/socket/ssl_client_socket_unittest.cc:9: #include <algorithm> On 2017/03/02 20:47:03, davidben wrote: > Nit: Swap this and the blank line below. Done. https://codereview.chromium.org/2593063003/diff/500001/net/socket/ssl_client_... net/socket/ssl_client_socket_unittest.cc:535: // drained. On 2017/03/02 20:47:03, davidben wrote: > The comment should probably be clearer that we're explicitly not supporting > these cases for simplicity. Done. https://codereview.chromium.org/2593063003/diff/500001/net/socket/ssl_client_... net/socket/ssl_client_socket_unittest.cc:537: CHECK_GE(len, (int)read_if_ready_buf_.size()); On 2017/03/02 20:47:03, davidben wrote: > Nit: static_cast<int>(read_if_ready_buf_.size()) Done. https://codereview.chromium.org/2593063003/diff/500001/net/socket/ssl_client_... net/socket/ssl_client_socket_unittest.cc:602: DCHECK_NE(-1, pending_read_buf_len_); On 2017/03/02 20:47:02, davidben wrote: > 601 and 602 are already checked a few lines above. > > Why does 600 need to move after the if? Done. https://codereview.chromium.org/2593063003/diff/500001/net/socket/ssl_client_... net/socket/ssl_client_socket_unittest.cc:1488: EXPECT_THAT(rv, IsError(ERR_CONNECTION_RESET)); On 2017/03/02 20:47:03, davidben wrote: > ReadAndWaitForCompletion? Done.
Great work! I really look forward to the reduced memory usage that this feature should bring. Sorry that I have so many comments, almost all of them are minor. https://codereview.chromium.org/2593063003/diff/560001/net/base/net_error_list.h File net/base/net_error_list.h (right): https://codereview.chromium.org/2593063003/diff/560001/net/base/net_error_lis... net/base/net_error_list.h:389: // Socket ReadIfReady support is not implemented. Do you plan to support ReadIfReady() on every Socket implementation? If so, add a TODO here to do that and to remove this error code. https://codereview.chromium.org/2593063003/diff/560001/net/base/net_error_lis... net/base/net_error_list.h:389: // Socket ReadIfReady support is not implemented. Maybe append ", Read() should be used instead." https://codereview.chromium.org/2593063003/diff/560001/net/socket/socket.cc File net/socket/socket.cc (right): https://codereview.chromium.org/2593063003/diff/560001/net/socket/socket.cc#n... net/socket/socket.cc:1: // Copyright 2016 The Chromium Authors. All rights reserved. It seems to me that this patch was created already in 2017, please consider updating year. Although honestly I do not know what is the rule if you started to work on the patch back in 2016 but only uploaded it in 2017. BTW tools/boilerplate.py will create new files for you with current year. https://codereview.chromium.org/2593063003/diff/560001/net/socket/socket.cc#n... net/socket/socket.cc:14: int Socket::ReadIfReady(IOBuffer* buf, Every other method in this class is pure virtual. Are you planning to remove default implementation to make this one pure virtual as well? (I know you already have follow-up CLs in progress.) In that case, consider adding a TODO here, unless you plan to do it very soon (and you already have the CL lined up and you want to save yourself the effort of resolving a merge conflict). https://codereview.chromium.org/2593063003/diff/560001/net/socket/socket.h File net/socket/socket.h (right): https://codereview.chromium.org/2593063003/diff/560001/net/socket/socket.h#ne... net/socket/socket.h:26: // Reads data, up to |buf_len| bytes, from the socket. The number of bytes Optional: add a note at the beginning of the comment about Read() holding on to |buf|. https://codereview.chromium.org/2593063003/diff/560001/net/socket/socket.h#ne... net/socket/socket.h:37: virtual int Read(IOBuffer* buf, int buf_len, <rant> I am unhappy with how a scoped_refptr is passed around as a raw pointer (produced by get() at the call site) just to be wrapped into another scoped_refptr inside the Read() implementation. This is bad mostly because the argument type does not reflect the semantics. If Read() took |buf| as scoped_refptr by value while ReadIfReady() took it by raw pointer, that would nicely reflect the semantical difference. This is also bad because it is made possible by scoped_refptr being invasively implemented (by ScopedRefPtr base class), which is not the only way: for example, std::shared_ptr is not invasive (refcounter is not inside the instance but in the wrapper), and taking a raw pointer and constructing a new std::shared_ptr with that would make it be freed twice, and free after free is an especially sad kind of use after free. So being able to pass this by raw pointer is an implementation detail. Anyway, a few months ago I looked into what it would take to change |buf| argument type to scoped_refptr, but there are so many implementations and call sites that it did not seem like it was worth the effort. Maybe we should revisit that to make the distinction between Read() and ReadIfReady() clearer? </rant> https://codereview.chromium.org/2593063003/diff/560001/net/socket/socket.h#ne... net/socket/socket.h:40: // Reads as much data as possible into |buf| without blocking. Default Optional: add a note at the beginning of the comment stating that ReadIfReady() does not hold on to |buf|. https://codereview.chromium.org/2593063003/diff/560001/net/socket/socket.h#ne... net/socket/socket.h:40: // Reads as much data as possible into |buf| without blocking. Default If it reads as much data as possible, why does it take a |buf_len| argument? Or do you mean it reads at most |buf_len| bytes? https://codereview.chromium.org/2593063003/diff/560001/net/socket/socket.h#ne... net/socket/socket.h:40: // Reads as much data as possible into |buf| without blocking. Default What happens if a caller calls ReadIfReady(), that returns asynchronously, then the caller calls Read()? Or the other way around? Is it undefined behavior? Should this be forbidden in comments here, or should it be enforced with DCHECKs in each implementation? https://codereview.chromium.org/2593063003/diff/560001/net/socket/socket.h#ne... net/socket/socket.h:44: // code if an error happens; If read cannot be completed synchronously, Optional: replace every semicolon with full stop to decrease average sentence length. This sometimes makes is easier to read. https://codereview.chromium.org/2593063003/diff/560001/net/socket/socket.h#ne... net/socket/socket.h:46: // invoked with OK when data can be read, at which point, caller can call Suppose ReadIfReady() returns ERR_IO_PENDING, later it calls |callback|, then caller calls ReadIfReady() again. Is it guaranteed at that point that it will return synchronously, or is it allowed to return ERR_IO_PENDING again? Maybe spell this out in the comment. https://codereview.chromium.org/2593063003/diff/560001/net/socket/socket.h#ne... net/socket/socket.h:49: // Note: if 0 is returned synchronously, it means that EOF is reached; if 0/OK Optional: move "if 0 is returned synchronously" into the text above by saying "Upon synchronous completion, returns the number of bytes read, or 0 on EOF, or an error code if an error happens.". https://codereview.chromium.org/2593063003/diff/560001/net/socket/socket.h#ne... net/socket/socket.h:50: // is returned asynchronously, it means that ReadIfReady() can be retried. Remove "0 is returned asynchronously" case because it is already explicitly spelled out above. https://codereview.chromium.org/2593063003/diff/560001/net/socket/socket_bio_... File net/socket/socket_bio_adapter.cc (right): https://codereview.chromium.org/2593063003/diff/560001/net/socket/socket_bio_... net/socket/socket_bio_adapter.cc:162: void SocketBIOAdapter::OnSocketReadIfReadyComplete(int result) { Should this callback not call socket_->ReadIfReady() again to complete the read? https://codereview.chromium.org/2593063003/diff/560001/net/socket/socket_bio_... net/socket/socket_bio_adapter.cc:166: // Do not use HandleSocketReadResult() because |result == OK| doesn't mean EOF End sentence with full stop. https://codereview.chromium.org/2593063003/diff/560001/net/socket/socket_bio_... File net/socket/socket_bio_adapter_unittest.cc (right): https://codereview.chromium.org/2593063003/diff/560001/net/socket/socket_bio_... net/socket/socket_bio_adapter_unittest.cc:32: // ReadIfReady() field trial is enabled, but ReadyIfReady() is unimplemented. SUPPORTED is implemented and NOT_SUPPORTED is unimplemented, not the other way around, right? https://codereview.chromium.org/2593063003/diff/560001/net/socket/socket_bio_... net/socket/socket_bio_adapter_unittest.cc:230: // After waiting, the data is available if Read() is used. Should SocketBIOAdapter not behave the same way with the new read? That is, after running the loop, data should be read into the buffer, just like before? https://codereview.chromium.org/2593063003/diff/560001/net/socket/socket_posi... File net/socket/socket_posix.cc (right): https://codereview.chromium.org/2593063003/diff/560001/net/socket/socket_posi... net/socket/socket_posix.cc:276: // Synchronous operation not supported What does this comment mean? Could it be removed? https://codereview.chromium.org/2593063003/diff/560001/net/socket/socket_posi... net/socket/socket_posix.cc:395: } else { // !read_if_ready_callback_.is_null() Optional: remove this comment, remove DCHECK from three lines above, add DCHECK(!read_if_ready_callback_.is_null()) on the next line. https://codereview.chromium.org/2593063003/diff/560001/net/socket/socket_posi... net/socket/socket_posix.cc:533: if (!read_callback_.is_null()) { I am wondering if |read_if_ready_callback_| would also need to be reset here. https://codereview.chromium.org/2593063003/diff/560001/net/socket/socket_posix.h File net/socket/socket_posix.h (right): https://codereview.chromium.org/2593063003/diff/560001/net/socket/socket_posi... net/socket/socket_posix.h:63: // Reads as much data as possible into |buf| without blocking. If read is to Optional: mention that no reference to |buf| is retained upon async return. https://codereview.chromium.org/2593063003/diff/560001/net/socket/socket_test... File net/socket/socket_test_util.cc (right): https://codereview.chromium.org/2593063003/diff/560001/net/socket/socket_test... net/socket/socket_test_util.cc:905: // Use base::Unretained() is safe because whenever callback is run "callback is run ..., it ..." implies that it is the callback that takes the weak pointer, whereas it is not. Please consider rewording. https://codereview.chromium.org/2593063003/diff/560001/net/socket/socket_test... net/socket/socket_test_util.cc:906: // asynchrously, it takes the weak ptr of the base class. Is it in MockClientSocket::RunCallbackAsync()? Can you please clarify in the comment, it took me some time to find it, sorry. https://codereview.chromium.org/2593063003/diff/560001/net/socket/socket_test... File net/socket/socket_test_util.h (right): https://codereview.chromium.org/2593063003/diff/560001/net/socket/socket_test... net/socket/socket_test_util.h:521: // If |enable_read_if_ready|, ReadIfReady() will be implemented. Move this comment to the line above |enable_read_if_ready_| member declaration. https://codereview.chromium.org/2593063003/diff/560001/net/socket/socket_test... net/socket/socket_test_util.h:521: // If |enable_read_if_ready|, ReadIfReady() will be implemented. "will be implemented" sounds strange, because it is implemented regardless of the value of this member (in the sense that it is part of the source code). "will be enabled", on the other hand, does not carry extra information with respect to the member name. Consider instead something like: "If true, ReadIfReady() is enabled; otherwise it returns ERR_READ_IF_READY_NOT_IMPLEMENTED." https://codereview.chromium.org/2593063003/diff/560001/net/socket/socket_test... net/socket/socket_test_util.h:662: // If |enable_read_if_ready|, ReadIfReady() will be implemented. Same comments as for the identically named method/member above. https://codereview.chromium.org/2593063003/diff/560001/net/socket/ssl_client_... File net/socket/ssl_client_socket_impl.cc (right): https://codereview.chromium.org/2593063003/diff/560001/net/socket/ssl_client_... net/socket/ssl_client_socket_impl.cc:1069: user_read_buf_ = nullptr; Nice! https://codereview.chromium.org/2593063003/diff/560001/net/socket/ssl_client_... net/socket/ssl_client_socket_impl.cc:1564: // When ReadIfReady() is used, skip DoPayloadRead(). Read() calls into ReadIfReady(), so ReadIfReady() is always used, is it not? https://codereview.chromium.org/2593063003/diff/560001/net/socket/ssl_client_... net/socket/ssl_client_socket_impl.cc:1564: // When ReadIfReady() is used, skip DoPayloadRead(). I'm sorry, I do not understand how changing |rv_read| skips DoPayloadRead(). Maybe consider clarifing? https://codereview.chromium.org/2593063003/diff/560001/net/socket/ssl_client_... File net/socket/ssl_client_socket_impl.h (right): https://codereview.chromium.org/2593063003/diff/560001/net/socket/ssl_client_... net/socket/ssl_client_socket_impl.h:165: int DoPayloadRead(IOBuffer* buf, int buf_len); Optional: add a comment about whether this method saves a reference to |buf|. (Optional because my understanding is that private methods are not part of the public API therefore it is not required to document them as rigorously.) https://codereview.chromium.org/2593063003/diff/560001/net/socket/ssl_client_... File net/socket/ssl_client_socket_unittest.cc (left): https://codereview.chromium.org/2593063003/diff/560001/net/socket/ssl_client_... net/socket/ssl_client_socket_unittest.cc:226: Optional: maybe keep this empty like for better readability. https://codereview.chromium.org/2593063003/diff/560001/net/socket/ssl_client_... File net/socket/ssl_client_socket_unittest.cc (right): https://codereview.chromium.org/2593063003/diff/560001/net/socket/ssl_client_... net/socket/ssl_client_socket_unittest.cc:288: return transport_->Read( Nice! https://codereview.chromium.org/2593063003/diff/560001/net/socket/ssl_client_... net/socket/ssl_client_socket_unittest.cc:306: // If ReadIfReady() is used. Read() can call into ReadIfReady() which can return asynchronously in which case ReadIfReady() was used but |user_read_buf_ != nullptr|. Is there a way to modify this comment? Maybe something along the lines of "ReadIfReady() already read from the buffer synchronously." (with which wording it should be after the |if|, not before). https://codereview.chromium.org/2593063003/diff/560001/net/socket/ssl_client_... net/socket/ssl_client_socket_unittest.cc:324: user_read_buf_ = nullptr; Nice! https://codereview.chromium.org/2593063003/diff/560001/net/socket/ssl_client_... net/socket/ssl_client_socket_unittest.cc:955: // If GetParam(), try ReadIfReady() and fall back to Read() if needed. I agree that introducing a two-entry enum would be an overkill for this test parameter. How about introducing a private const bool enable_read_if_ready_ member in SSLClientSocketReadTest, initializing it in the constructor initializer list with GetParam(), and using that instead of GetParam() everywhere else. (This advice is inspired by https://big.corp.google.com/~jmcmaster/testing/2017/02/episode-465-to-comment...) Then this comment could be moved to right above that member declaration. https://codereview.chromium.org/2593063003/diff/560001/net/socket/ssl_client_... net/socket/ssl_client_socket_unittest.cc:3775: // TODO(xunjieli): crbug.com/690915. Implement ReadIfReady() for windows. Optional: use https:// prefix for crbug, because some viewers/editors need that to make this a clickable link (for example, gnome-terminal, when text is output using cat, less, diff, grep etc.). https://codereview.chromium.org/2593063003/diff/560001/net/socket/tcp_client_... File net/socket/tcp_client_socket.cc (right): https://codereview.chromium.org/2593063003/diff/560001/net/socket/tcp_client_... net/socket/tcp_client_socket.cc:103: int TCPClientSocket::ReadCommon(IOBuffer* buf, Consider keeping methods in the same order in the .h and .cc files. https://codereview.chromium.org/2593063003/diff/560001/net/socket/tcp_client_... net/socket/tcp_client_socket.cc:110: // |socket_| is gone. Therefore, it is safe to use base::Unretained() here. Optional: Can you please elaborate on "the callback won't be run once |socket_| is gone"? Is the callback cancelled every time the |socket_| is closed? (What about lines 157 and 211?) Is the callback cancelled in the destructor? This is optional because this comment is pre-existing. https://codereview.chromium.org/2593063003/diff/560001/net/spdy/spdy_session.cc File net/spdy/spdy_session.cc (right): https://codereview.chromium.org/2593063003/diff/560001/net/spdy/spdy_session.... net/spdy/spdy_session.cc:1809: int SpdySession::DoRead() { Oh no, I just reordered methods in https://crrev.com/2683643002 so that the order is consistent between spdy_session.h and spdy_session.cc. If you want to swap DoRead() and DoReadLoop(), could you please also do that in the header file? https://codereview.chromium.org/2593063003/diff/560001/net/spdy/spdy_session.... net/spdy/spdy_session.cc:1813: CHECK(connection_->socket()); Do you think this would flow more naturally? if (base::FeatureList::IsEnabled(Socket::kReadIfReadyExperiment)) { int rv = connection_->socket()->ReadIfReady( read_buffer_.get(), kReadBufferSize, base::Bind(&SpdySession::PumpReadLoop, weak_factory_.GetWeakPtr(), READ_STATE_DO_READ)); // TODO(xunjieli): Follow-up CL to release |read_buffer_|. // https://crbug.com/524258 if (rv == ERR_IO_PENDING) { read_state_ = READ_STATE_DO_READ; return rv; } if (rv != ERR_READ_IF_READY_NOT_IMPLEMENTED) { read_state_ = READ_STATE_DO_READ_COMPLETE; return rv; } } // Fallback to regular Read(). read_state_ = READ_STATE_DO_READ_COMPLETE; return connection_->socket()->Read( read_buffer_.get(), kReadBufferSize, base::Bind(&SpdySession::PumpReadLoop, weak_factory_.GetWeakPtr(), READ_STATE_DO_READ_COMPLETE)); https://codereview.chromium.org/2593063003/diff/560001/net/spdy/spdy_session.... net/spdy/spdy_session.cc:1821: // TODO(xunjieli): Follow-up CL to release |read_buffer_|. crbug.com/524258. Optional: use https:// prefix for crbug, because some viewers/editors need that to make this a clickable link (for example, gnome-terminal, when text is output using cat, less, diff, grep etc.). https://codereview.chromium.org/2593063003/diff/560001/net/spdy/spdy_session_... File net/spdy/spdy_session_unittest.cc (right): https://codereview.chromium.org/2593063003/diff/560001/net/spdy/spdy_session_... net/spdy/spdy_session_unittest.cc:5431: // ReadIfReady() field trial is enabled, but ReadyIfReady() is unimplemented. SUPPORTED is implemented and NOT_SUPPORTED is unimplemented, not the other way around, right? https://codereview.chromium.org/2593063003/diff/560001/net/spdy/spdy_session_... net/spdy/spdy_session_unittest.cc:5439: class SpdySessionReadyIfReadyTest s/SpdySessionReadyIfReadyTest/SpdySessionReadIfReadyTest/
Thanks, Bence. PTAL. https://codereview.chromium.org/2593063003/diff/560001/net/base/net_error_list.h File net/base/net_error_list.h (right): https://codereview.chromium.org/2593063003/diff/560001/net/base/net_error_lis... net/base/net_error_list.h:389: // Socket ReadIfReady support is not implemented. On 2017/03/03 16:33:40, Bence wrote: > Do you plan to support ReadIfReady() on every Socket implementation? If so, add > a TODO here to do that and to remove this error code. Nope. We aren't going to support every Socket implementation. There are a lot out there. We are only going to implement TCP socket on posix and on windows for this particular experiment. https://codereview.chromium.org/2593063003/diff/560001/net/base/net_error_lis... net/base/net_error_list.h:389: // Socket ReadIfReady support is not implemented. On 2017/03/03 16:33:40, Bence wrote: > Maybe append ", Read() should be used instead." Done. https://codereview.chromium.org/2593063003/diff/560001/net/socket/socket.cc File net/socket/socket.cc (right): https://codereview.chromium.org/2593063003/diff/560001/net/socket/socket.cc#n... net/socket/socket.cc:1: // Copyright 2016 The Chromium Authors. All rights reserved. On 2017/03/03 16:33:40, Bence wrote: > It seems to me that this patch was created already in 2017, please consider > updating year. Although honestly I do not know what is the rule if you started > to work on the patch back in 2016 but only uploaded it in 2017. > > BTW tools/boilerplate.py will create new files for you with current year. Done. https://codereview.chromium.org/2593063003/diff/560001/net/socket/socket.cc#n... net/socket/socket.cc:14: int Socket::ReadIfReady(IOBuffer* buf, On 2017/03/03 16:33:40, Bence wrote: > Every other method in this class is pure virtual. Are you planning to remove > default implementation to make this one pure virtual as well? (I know you > already have follow-up CLs in progress.) In that case, consider adding a TODO > here, unless you plan to do it very soon (and you already have the CL lined up > and you want to save yourself the effort of resolving a merge conflict). No, we are keeping the default implementation. I don't think we should inline it right? https://codereview.chromium.org/2593063003/diff/560001/net/socket/socket.h File net/socket/socket.h (right): https://codereview.chromium.org/2593063003/diff/560001/net/socket/socket.h#ne... net/socket/socket.h:26: // Reads data, up to |buf_len| bytes, from the socket. The number of bytes On 2017/03/03 16:33:40, Bence wrote: > Optional: add a note at the beginning of the comment about Read() holding on to > |buf|. There is a comment about this actually in the paragraph (see below). I will keep it this way. If the // operation is not completed immediately, the socket acquires a reference to // the provided buffer until the callback is invoked or the socket is // closed. https://codereview.chromium.org/2593063003/diff/560001/net/socket/socket.h#ne... net/socket/socket.h:37: virtual int Read(IOBuffer* buf, int buf_len, On 2017/03/03 16:33:40, Bence wrote: > <rant> > > I am unhappy with how a scoped_refptr is passed around as a raw pointer > (produced by get() at the call site) just to be wrapped into another > scoped_refptr inside the Read() implementation. This is bad mostly because the > argument type does not reflect the semantics. If Read() took |buf| as > scoped_refptr by value while ReadIfReady() took it by raw pointer, that would > nicely reflect the semantical difference. > > This is also bad because it is made possible by scoped_refptr being invasively > implemented (by ScopedRefPtr base class), which is not the only way: for > example, std::shared_ptr is not invasive (refcounter is not inside the instance > but in the wrapper), and taking a raw pointer and constructing a new > std::shared_ptr with that would make it be freed twice, and free after free is > an especially sad kind of use after free. So being able to pass this by raw > pointer is an implementation detail. > > Anyway, a few months ago I looked into what it would take to change |buf| > argument type to scoped_refptr, but there are so many implementations and call > sites that it did not seem like it was worth the effort. Maybe we should > revisit that to make the distinction between Read() and ReadIfReady() clearer? > > </rant> Acknowledged. Rant acknowledged :) https://codereview.chromium.org/2593063003/diff/560001/net/socket/socket.h#ne... net/socket/socket.h:40: // Reads as much data as possible into |buf| without blocking. Default On 2017/03/03 16:33:40, Bence wrote: > Optional: add a note at the beginning of the comment stating that ReadIfReady() > does not hold on to |buf|. I will keep it this way. I have a comment in the later section about synchronous completion and asynchronous completion. I am having some trouble reorganizing the paragraph so that we can move the async part to the very front. I don't want to give the impression that we always hold on to |buf|. It should only happen in the async case for Read(). https://codereview.chromium.org/2593063003/diff/560001/net/socket/socket.h#ne... net/socket/socket.h:40: // Reads as much data as possible into |buf| without blocking. Default On 2017/03/03 16:33:40, Bence wrote: > What happens if a caller calls ReadIfReady(), that returns asynchronously, then > the caller calls Read()? Or the other way around? Is it undefined behavior? > Should this be forbidden in comments here, or should it be enforced with DCHECKs > in each implementation? It should be the same as calling Read() twice when the first one didn't complete. Yes, it's enforced by each implementation. I don't think we should enforce any restriction on the interface level, because some implementations Read() and ReadIfReady() can be used together, and some (e.g. FakeBlockingStreamSocket) can't. https://codereview.chromium.org/2593063003/diff/560001/net/socket/socket.h#ne... net/socket/socket.h:40: // Reads as much data as possible into |buf| without blocking. Default On 2017/03/03 16:33:40, Bence wrote: > If it reads as much data as possible, why does it take a |buf_len| argument? Or > do you mean it reads at most |buf_len| bytes? Yep, it reads at most |buf_len_|. Clarified in the doc. Done. https://codereview.chromium.org/2593063003/diff/560001/net/socket/socket.h#ne... net/socket/socket.h:44: // code if an error happens; If read cannot be completed synchronously, On 2017/03/03 16:33:40, Bence wrote: > Optional: replace every semicolon with full stop to decrease average sentence > length. This sometimes makes is easier to read. Done. https://codereview.chromium.org/2593063003/diff/560001/net/socket/socket.h#ne... net/socket/socket.h:46: // invoked with OK when data can be read, at which point, caller can call On 2017/03/03 16:33:40, Bence wrote: > Suppose ReadIfReady() returns ERR_IO_PENDING, later it calls |callback|, then > caller calls ReadIfReady() again. Is it guaranteed at that point that it will > return synchronously, or is it allowed to return ERR_IO_PENDING again? Maybe > spell this out in the comment. Nope. it's not guaranteed. It's legal for an implementation to return ERR_IO_PENDING repeatedly. https://codereview.chromium.org/2593063003/diff/560001/net/socket/socket.h#ne... net/socket/socket.h:49: // Note: if 0 is returned synchronously, it means that EOF is reached; if 0/OK On 2017/03/03 16:33:40, Bence wrote: > Optional: move "if 0 is returned synchronously" into the text above by saying > "Upon synchronous completion, returns the number of bytes read, or 0 on EOF, or > an error code if an error happens.". Done. https://codereview.chromium.org/2593063003/diff/560001/net/socket/socket.h#ne... net/socket/socket.h:50: // is returned asynchronously, it means that ReadIfReady() can be retried. On 2017/03/03 16:33:40, Bence wrote: > Remove "0 is returned asynchronously" case because it is already explicitly > spelled out above. Done. https://codereview.chromium.org/2593063003/diff/560001/net/socket/socket_bio_... File net/socket/socket_bio_adapter.cc (right): https://codereview.chromium.org/2593063003/diff/560001/net/socket/socket_bio_... net/socket/socket_bio_adapter.cc:162: void SocketBIOAdapter::OnSocketReadIfReadyComplete(int result) { On 2017/03/03 16:33:40, Bence wrote: > Should this callback not call socket_->ReadIfReady() again to complete the read? Nope. The user of SocketBIOAdapter, SSLClientSocketImpl, is expected to retry the read. https://codereview.chromium.org/2593063003/diff/560001/net/socket/socket_bio_... net/socket/socket_bio_adapter.cc:166: // Do not use HandleSocketReadResult() because |result == OK| doesn't mean EOF On 2017/03/03 16:33:40, Bence wrote: > End sentence with full stop. Done. https://codereview.chromium.org/2593063003/diff/560001/net/socket/socket_bio_... File net/socket/socket_bio_adapter_unittest.cc (right): https://codereview.chromium.org/2593063003/diff/560001/net/socket/socket_bio_... net/socket/socket_bio_adapter_unittest.cc:32: // ReadIfReady() field trial is enabled, but ReadyIfReady() is unimplemented. On 2017/03/03 16:33:40, Bence wrote: > SUPPORTED is implemented and NOT_SUPPORTED is unimplemented, not the other way > around, right? Done. Good catch! https://codereview.chromium.org/2593063003/diff/560001/net/socket/socket_bio_... net/socket/socket_bio_adapter_unittest.cc:230: // After waiting, the data is available if Read() is used. On 2017/03/03 16:33:40, Bence wrote: > Should SocketBIOAdapter not behave the same way with the new read? That is, > after running the loop, data should be read into the buffer, just like before? Nope. For the new ReadIfReady(), only synchronous completion will put data into the buffer. Any asynchronous completion will just notify the user that the user can retry ReadIfReady because data is *likely* to be available. https://codereview.chromium.org/2593063003/diff/560001/net/socket/socket_posi... File net/socket/socket_posix.cc (right): https://codereview.chromium.org/2593063003/diff/560001/net/socket/socket_posi... net/socket/socket_posix.cc:276: // Synchronous operation not supported On 2017/03/03 16:33:41, Bence wrote: > What does this comment mean? Could it be removed? Done. Removed. I think having a callback as a requirement is an idiom now, so this comment isn't as meaningful. Maybe they wanted to add a Read() method where you can pass in a null callback to force synchronous completion.. https://codereview.chromium.org/2593063003/diff/560001/net/socket/socket_posi... net/socket/socket_posix.cc:395: } else { // !read_if_ready_callback_.is_null() On 2017/03/03 16:33:41, Bence wrote: > Optional: remove this comment, remove DCHECK from three lines above, add > DCHECK(!read_if_ready_callback_.is_null()) on the next line. Done. https://codereview.chromium.org/2593063003/diff/560001/net/socket/socket_posi... net/socket/socket_posix.cc:533: if (!read_callback_.is_null()) { On 2017/03/03 16:33:41, Bence wrote: > I am wondering if |read_if_ready_callback_| would also need to be reset here. Done. Good point. https://codereview.chromium.org/2593063003/diff/560001/net/socket/socket_posix.h File net/socket/socket_posix.h (right): https://codereview.chromium.org/2593063003/diff/560001/net/socket/socket_posi... net/socket/socket_posix.h:63: // Reads as much data as possible into |buf| without blocking. If read is to On 2017/03/03 16:33:41, Bence wrote: > Optional: mention that no reference to |buf| is retained upon async return. Done. https://codereview.chromium.org/2593063003/diff/560001/net/socket/socket_test... File net/socket/socket_test_util.cc (right): https://codereview.chromium.org/2593063003/diff/560001/net/socket/socket_test... net/socket/socket_test_util.cc:905: // Use base::Unretained() is safe because whenever callback is run On 2017/03/03 16:33:41, Bence wrote: > "callback is run ..., it ..." implies that it is the callback that takes the > weak pointer, whereas it is not. Please consider rewording. Done. https://codereview.chromium.org/2593063003/diff/560001/net/socket/socket_test... net/socket/socket_test_util.cc:906: // asynchrously, it takes the weak ptr of the base class. On 2017/03/03 16:33:41, Bence wrote: > Is it in MockClientSocket::RunCallbackAsync()? Can you please clarify in the > comment, it took me some time to find it, sorry. Done. https://codereview.chromium.org/2593063003/diff/560001/net/socket/socket_test... File net/socket/socket_test_util.h (right): https://codereview.chromium.org/2593063003/diff/560001/net/socket/socket_test... net/socket/socket_test_util.h:521: // If |enable_read_if_ready|, ReadIfReady() will be implemented. On 2017/03/03 16:33:41, Bence wrote: > "will be implemented" sounds strange, because it is implemented regardless of > the value of this member (in the sense that it is part of the source code). > "will be enabled", on the other hand, does not carry extra information with > respect to the member name. Consider instead something like: "If true, > ReadIfReady() is enabled; otherwise it returns > ERR_READ_IF_READY_NOT_IMPLEMENTED." Done. https://codereview.chromium.org/2593063003/diff/560001/net/socket/socket_test... net/socket/socket_test_util.h:521: // If |enable_read_if_ready|, ReadIfReady() will be implemented. On 2017/03/03 16:33:41, Bence wrote: > Move this comment to the line above |enable_read_if_ready_| member declaration. Done. https://codereview.chromium.org/2593063003/diff/560001/net/socket/socket_test... net/socket/socket_test_util.h:662: // If |enable_read_if_ready|, ReadIfReady() will be implemented. On 2017/03/03 16:33:41, Bence wrote: > Same comments as for the identically named method/member above. Done. https://codereview.chromium.org/2593063003/diff/560001/net/socket/ssl_client_... File net/socket/ssl_client_socket_impl.cc (right): https://codereview.chromium.org/2593063003/diff/560001/net/socket/ssl_client_... net/socket/ssl_client_socket_impl.cc:1069: user_read_buf_ = nullptr; On 2017/03/03 16:33:41, Bence wrote: > Nice! Acknowledged. https://codereview.chromium.org/2593063003/diff/560001/net/socket/ssl_client_... net/socket/ssl_client_socket_impl.cc:1564: // When ReadIfReady() is used, skip DoPayloadRead(). On 2017/03/03 16:33:41, Bence wrote: > I'm sorry, I do not understand how changing |rv_read| skips DoPayloadRead(). > Maybe consider clarifing? Done. https://codereview.chromium.org/2593063003/diff/560001/net/socket/ssl_client_... net/socket/ssl_client_socket_impl.cc:1564: // When ReadIfReady() is used, skip DoPayloadRead(). On 2017/03/03 16:33:41, Bence wrote: > Read() calls into ReadIfReady(), so ReadIfReady() is always used, is it not? Done. Clarified in the comment. Hopefully it's clearer? https://codereview.chromium.org/2593063003/diff/560001/net/socket/ssl_client_... File net/socket/ssl_client_socket_impl.h (right): https://codereview.chromium.org/2593063003/diff/560001/net/socket/ssl_client_... net/socket/ssl_client_socket_impl.h:165: int DoPayloadRead(IOBuffer* buf, int buf_len); On 2017/03/03 16:33:41, Bence wrote: > Optional: add a comment about whether this method saves a reference to |buf|. > (Optional because my understanding is that private methods are not part of the > public API therefore it is not required to document them as rigorously.) Acknowledged. https://codereview.chromium.org/2593063003/diff/560001/net/socket/ssl_client_... File net/socket/ssl_client_socket_unittest.cc (left): https://codereview.chromium.org/2593063003/diff/560001/net/socket/ssl_client_... net/socket/ssl_client_socket_unittest.cc:226: On 2017/03/03 16:33:41, Bence wrote: > Optional: maybe keep this empty like for better readability. Done. https://codereview.chromium.org/2593063003/diff/560001/net/socket/ssl_client_... File net/socket/ssl_client_socket_unittest.cc (right): https://codereview.chromium.org/2593063003/diff/560001/net/socket/ssl_client_... net/socket/ssl_client_socket_unittest.cc:288: return transport_->Read( On 2017/03/03 16:33:41, Bence wrote: > Nice! Acknowledged. https://codereview.chromium.org/2593063003/diff/560001/net/socket/ssl_client_... net/socket/ssl_client_socket_unittest.cc:306: // If ReadIfReady() is used. On 2017/03/03 16:33:41, Bence wrote: > Read() can call into ReadIfReady() which can return asynchronously in which case > ReadIfReady() was used but |user_read_buf_ != nullptr|. Is there a way to > modify this comment? Maybe something along the lines of "ReadIfReady() already > read from the buffer synchronously." (with which wording it should be after the > |if|, not before). Done. https://codereview.chromium.org/2593063003/diff/560001/net/socket/ssl_client_... net/socket/ssl_client_socket_unittest.cc:324: user_read_buf_ = nullptr; On 2017/03/03 16:33:41, Bence wrote: > Nice! Acknowledged. https://codereview.chromium.org/2593063003/diff/560001/net/socket/ssl_client_... net/socket/ssl_client_socket_unittest.cc:955: // If GetParam(), try ReadIfReady() and fall back to Read() if needed. On 2017/03/03 16:33:41, Bence wrote: > I agree that introducing a two-entry enum would be an overkill for this test > parameter. How about introducing a private const bool enable_read_if_ready_ > member in SSLClientSocketReadTest, initializing it in the constructor > initializer list with GetParam(), and using that instead of GetParam() > everywhere else. (This advice is inspired by > https://big.corp.google.com/~jmcmaster/testing/2017/02/episode-465-to-comment...) > > Then this comment could be moved to right above that member declaration. Done. https://codereview.chromium.org/2593063003/diff/560001/net/socket/ssl_client_... net/socket/ssl_client_socket_unittest.cc:3775: // TODO(xunjieli): crbug.com/690915. Implement ReadIfReady() for windows. On 2017/03/03 16:33:41, Bence wrote: > Optional: use https:// prefix for crbug, because some viewers/editors need that > to make this a clickable link (for example, gnome-terminal, when text is output > using cat, less, diff, grep etc.). Done. https://codereview.chromium.org/2593063003/diff/560001/net/socket/tcp_client_... File net/socket/tcp_client_socket.cc (right): https://codereview.chromium.org/2593063003/diff/560001/net/socket/tcp_client_... net/socket/tcp_client_socket.cc:103: int TCPClientSocket::ReadCommon(IOBuffer* buf, On 2017/03/03 16:33:41, Bence wrote: > Consider keeping methods in the same order in the .h and .cc files. The thing is that the order is not maintained in this file :( I tried to match the order by having ReadCommon() above DoConnectLoop() in both h and cc. https://codereview.chromium.org/2593063003/diff/560001/net/socket/tcp_client_... net/socket/tcp_client_socket.cc:110: // |socket_| is gone. Therefore, it is safe to use base::Unretained() here. On 2017/03/03 16:33:41, Bence wrote: > Optional: Can you please elaborate on "the callback won't be run once |socket_| > is gone"? Is the callback cancelled every time the |socket_| is closed? (What > about lines 157 and 211?) Is the callback cancelled in the destructor? > > This is optional because this comment is pre-existing. The callback won't be run because the underlying socket won't callback to us since it's already gone/closed. I added the "closed" case in the comment. Hopefully that's clearer? https://codereview.chromium.org/2593063003/diff/560001/net/spdy/spdy_session.cc File net/spdy/spdy_session.cc (right): https://codereview.chromium.org/2593063003/diff/560001/net/spdy/spdy_session.... net/spdy/spdy_session.cc:1809: int SpdySession::DoRead() { On 2017/03/03 16:33:41, Bence wrote: > Oh no, I just reordered methods in https://crrev.com/2683643002 so that the > order is consistent between spdy_session.h and spdy_session.cc. If you want to > swap DoRead() and DoReadLoop(), could you please also do that in the header > file? Done. https://codereview.chromium.org/2593063003/diff/560001/net/spdy/spdy_session.... net/spdy/spdy_session.cc:1813: CHECK(connection_->socket()); On 2017/03/03 16:33:42, Bence wrote: > Do you think this would flow more naturally? > > if (base::FeatureList::IsEnabled(Socket::kReadIfReadyExperiment)) { > int rv = connection_->socket()->ReadIfReady( > read_buffer_.get(), kReadBufferSize, > base::Bind(&SpdySession::PumpReadLoop, weak_factory_.GetWeakPtr(), > READ_STATE_DO_READ)); > // TODO(xunjieli): Follow-up CL to release |read_buffer_|. > // https://crbug.com/524258 > if (rv == ERR_IO_PENDING) { > read_state_ = READ_STATE_DO_READ; > return rv; > } > if (rv != ERR_READ_IF_READY_NOT_IMPLEMENTED) { > read_state_ = READ_STATE_DO_READ_COMPLETE; > return rv; > } > } > // Fallback to regular Read(). > read_state_ = READ_STATE_DO_READ_COMPLETE; > return connection_->socket()->Read( > read_buffer_.get(), kReadBufferSize, > base::Bind(&SpdySession::PumpReadLoop, weak_factory_.GetWeakPtr(), > READ_STATE_DO_READ_COMPLETE)); I may be biased, but I like my verson better. Fewer lines of code and the state change is easier to read -- default to READ_STATE_DO_READ_COMPLETED but for async ReadIfReady() case go to READ_STATE_DO_READ. https://codereview.chromium.org/2593063003/diff/560001/net/spdy/spdy_session.... net/spdy/spdy_session.cc:1821: // TODO(xunjieli): Follow-up CL to release |read_buffer_|. crbug.com/524258. On 2017/03/03 16:33:42, Bence wrote: > Optional: use https:// prefix for crbug, because some viewers/editors need that > to make this a clickable link (for example, gnome-terminal, when text is output > using cat, less, diff, grep etc.). Done. I personally avoid the https:// prefix because that's extra 8 characters and makes one line into two. https://codereview.chromium.org/2593063003/diff/560001/net/spdy/spdy_session_... File net/spdy/spdy_session_unittest.cc (right): https://codereview.chromium.org/2593063003/diff/560001/net/spdy/spdy_session_... net/spdy/spdy_session_unittest.cc:5431: // ReadIfReady() field trial is enabled, but ReadyIfReady() is unimplemented. On 2017/03/03 16:33:42, Bence wrote: > SUPPORTED is implemented and NOT_SUPPORTED is unimplemented, not the other way > around, right? Done. https://codereview.chromium.org/2593063003/diff/560001/net/spdy/spdy_session_... net/spdy/spdy_session_unittest.cc:5439: class SpdySessionReadyIfReadyTest On 2017/03/03 16:33:42, Bence wrote: > s/SpdySessionReadyIfReadyTest/SpdySessionReadIfReadyTest/ Done. Oops.
LGTM, thank you. https://codereview.chromium.org/2593063003/diff/560001/net/socket/socket.cc File net/socket/socket.cc (right): https://codereview.chromium.org/2593063003/diff/560001/net/socket/socket.cc#n... net/socket/socket.cc:14: int Socket::ReadIfReady(IOBuffer* buf, On 2017/03/03 19:41:05, xunjieli wrote: > On 2017/03/03 16:33:40, Bence wrote: > > Every other method in this class is pure virtual. Are you planning to remove > > default implementation to make this one pure virtual as well? (I know you > > already have follow-up CLs in progress.) In that case, consider adding a TODO > > here, unless you plan to do it very soon (and you already have the CL lined up > > and you want to save yourself the effort of resolving a merge conflict). > > No, we are keeping the default implementation. I don't think we should inline it > right? No for inlining. I believe Chromium enforces that no virtual methods can be inlined (by a clang plugin). https://codereview.chromium.org/2593063003/diff/560001/net/socket/tcp_client_... File net/socket/tcp_client_socket.cc (right): https://codereview.chromium.org/2593063003/diff/560001/net/socket/tcp_client_... net/socket/tcp_client_socket.cc:110: // |socket_| is gone. Therefore, it is safe to use base::Unretained() here. On 2017/03/03 19:41:06, xunjieli wrote: > On 2017/03/03 16:33:41, Bence wrote: > > Optional: Can you please elaborate on "the callback won't be run once > |socket_| > > is gone"? Is the callback cancelled every time the |socket_| is closed? > (What > > about lines 157 and 211?) Is the callback cancelled in the destructor? > > > > This is optional because this comment is pre-existing. > > The callback won't be run because the underlying socket won't callback to us > since it's already gone/closed. I added the "closed" case in the comment. > Hopefully that's clearer? Yes, thank you. https://codereview.chromium.org/2593063003/diff/560001/net/spdy/spdy_session.cc File net/spdy/spdy_session.cc (right): https://codereview.chromium.org/2593063003/diff/560001/net/spdy/spdy_session.... net/spdy/spdy_session.cc:1813: CHECK(connection_->socket()); On 2017/03/03 19:41:06, xunjieli wrote: > On 2017/03/03 16:33:42, Bence wrote: > > Do you think this would flow more naturally? > > > > if (base::FeatureList::IsEnabled(Socket::kReadIfReadyExperiment)) { > > int rv = connection_->socket()->ReadIfReady( > > read_buffer_.get(), kReadBufferSize, > > base::Bind(&SpdySession::PumpReadLoop, weak_factory_.GetWeakPtr(), > > READ_STATE_DO_READ)); > > // TODO(xunjieli): Follow-up CL to release |read_buffer_|. > > // https://crbug.com/524258 > > if (rv == ERR_IO_PENDING) { > > read_state_ = READ_STATE_DO_READ; > > return rv; > > } > > if (rv != ERR_READ_IF_READY_NOT_IMPLEMENTED) { > > read_state_ = READ_STATE_DO_READ_COMPLETE; > > return rv; > > } > > } > > // Fallback to regular Read(). > > read_state_ = READ_STATE_DO_READ_COMPLETE; > > return connection_->socket()->Read( > > read_buffer_.get(), kReadBufferSize, > > base::Bind(&SpdySession::PumpReadLoop, weak_factory_.GetWeakPtr(), > > READ_STATE_DO_READ_COMPLETE)); > > I may be biased, but I like my verson better. Fewer lines of code and the state > change is easier to read -- default to READ_STATE_DO_READ_COMPLETED but for > async ReadIfReady() case go to READ_STATE_DO_READ. It's okay to be biased, you are the author. :) https://codereview.chromium.org/2593063003/diff/560001/net/spdy/spdy_session.... net/spdy/spdy_session.cc:1821: // TODO(xunjieli): Follow-up CL to release |read_buffer_|. crbug.com/524258. On 2017/03/03 19:41:06, xunjieli wrote: > On 2017/03/03 16:33:42, Bence wrote: > > Optional: use https:// prefix for crbug, because some viewers/editors need > that > > to make this a clickable link (for example, gnome-terminal, when text is > output > > using cat, less, diff, grep etc.). > > Done. I personally avoid the https:// prefix because that's extra 8 characters > and makes one line into two. I guess then you are not using vim in gnome-terminal to write code. :) https://codereview.chromium.org/2593063003/diff/580001/net/socket/ssl_client_... File net/socket/ssl_client_socket_impl.cc (right): https://codereview.chromium.org/2593063003/diff/580001/net/socket/ssl_client_... net/socket/ssl_client_socket_impl.cc:1566: // the user know that read can be retried. Ah, that makes sense, thanks! Since "When |user_read_buf_| is null and |user_read_callback_| isn't" is redundant with code, I think you can just write "ReadIfReady() was called by the user" without repeating the if conditions.
Thanks! https://codereview.chromium.org/2593063003/diff/580001/net/socket/ssl_client_... File net/socket/ssl_client_socket_impl.cc (right): https://codereview.chromium.org/2593063003/diff/580001/net/socket/ssl_client_... net/socket/ssl_client_socket_impl.cc:1566: // the user know that read can be retried. On 2017/03/06 23:43:14, Bence wrote: > Ah, that makes sense, thanks! > > Since "When |user_read_buf_| is null and |user_read_callback_| isn't" is > redundant with code, I think you can just write "ReadIfReady() was called by the > user" without repeating the if conditions. Done.
The CQ bit was checked by xunjieli@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from davidben@chromium.org, bnc@chromium.org Link to the patchset: https://codereview.chromium.org/2593063003/#ps600001 (title: "Address Bence comment")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
The CQ bit was checked by xunjieli@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
On 2017/03/07 15:09:40, commit-bot: I haz the power wrote: > Try jobs failed on following builders: > win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, > http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...) the test_installer failure is a flake... Happened to others who were trying to land Cls. Retrying...
The CQ bit was checked by xunjieli@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Failed to apply patch for net/spdy/spdy_session.cc:
While running git apply --index -p1;
error: patch failed: net/spdy/spdy_session.cc:11
error: net/spdy/spdy_session.cc: patch does not apply
Patch: net/spdy/spdy_session.cc
Index: net/spdy/spdy_session.cc
diff --git a/net/spdy/spdy_session.cc b/net/spdy/spdy_session.cc
index
431022624477c12cdb079655abc77cb9b7723f39..79257d7795f2247d66cea021942970f24158d888
100644
--- a/net/spdy/spdy_session.cc
+++ b/net/spdy/spdy_session.cc
@@ -11,6 +11,7 @@
#include "base/bind.h"
#include "base/compiler_specific.h"
+#include "base/feature_list.h"
#include "base/location.h"
#include "base/logging.h"
#include "base/memory/ptr_util.h"
@@ -45,6 +46,7 @@
#include "net/log/net_log_source_type.h"
#include "net/log/net_log_with_source.h"
#include "net/proxy/proxy_server.h"
+#include "net/socket/socket.h"
#include "net/socket/ssl_client_socket.h"
#include "net/spdy/platform/api/spdy_estimate_memory_usage.h"
#include "net/spdy/spdy_buffer_producer.h"
@@ -1863,10 +1865,25 @@ int SpdySession::DoRead() {
CHECK(connection_);
CHECK(connection_->socket());
read_state_ = READ_STATE_DO_READ_COMPLETE;
- return connection_->socket()->Read(
- read_buffer_.get(), kReadBufferSize,
- base::Bind(&SpdySession::PumpReadLoop, weak_factory_.GetWeakPtr(),
- READ_STATE_DO_READ_COMPLETE));
+ int rv = ERR_READ_IF_READY_NOT_IMPLEMENTED;
+ if (base::FeatureList::IsEnabled(Socket::kReadIfReadyExperiment)) {
+ rv = connection_->socket()->ReadIfReady(
+ read_buffer_.get(), kReadBufferSize,
+ base::Bind(&SpdySession::PumpReadLoop, weak_factory_.GetWeakPtr(),
+ READ_STATE_DO_READ));
+ // TODO(xunjieli): Follow-up CL to release |read_buffer_|.
+ // https://crbug.com/690915.
+ }
+ if (rv == ERR_READ_IF_READY_NOT_IMPLEMENTED) {
+ // Fallback to regular Read().
+ return connection_->socket()->Read(
+ read_buffer_.get(), kReadBufferSize,
+ base::Bind(&SpdySession::PumpReadLoop, weak_factory_.GetWeakPtr(),
+ READ_STATE_DO_READ_COMPLETE));
+ }
+ if (rv == ERR_IO_PENDING)
+ read_state_ = READ_STATE_DO_READ;
+ return rv;
}
int SpdySession::DoReadComplete(int result) {
The CQ bit was checked by xunjieli@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from bnc@chromium.org, davidben@chromium.org Link to the patchset: https://codereview.chromium.org/2593063003/#ps620001 (title: "Rebased")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 620001, "attempt_start_ts": 1488907954785700,
"parent_rev": "f4ac7d77d34333b0873a799dc17097818b82d589", "commit_rev":
"321a96f376e57651a1ad987f5585c0f07128e63a"}
CQ is committing da patch.
Bot data: {"patchset_id": 620001, "attempt_start_ts": 1488907954785700,
"parent_rev": "f4ac7d77d34333b0873a799dc17097818b82d589", "commit_rev":
"321a96f376e57651a1ad987f5585c0f07128e63a"}
Message was sent while issue was closed.
Description was changed from ========== Add Socket::ReadIfReady() This CL adds a new read method to socket class. The new read method allows caller to not retain an IOBuffer if desired. The new functionality is hidden behind a field trial flag: SocketReadIfReady. The design doc is at: https://docs.google.com/document/d/1DsVcQ4-jsaIhu_OvIid-UHMUQfFqWYfmThMnQoZ1c... BUG=690915 ========== to ========== Add Socket::ReadIfReady() This CL adds a new read method to socket class. The new read method allows caller to not retain an IOBuffer if desired. The new functionality is hidden behind a field trial flag: SocketReadIfReady. The design doc is at: https://docs.google.com/document/d/1DsVcQ4-jsaIhu_OvIid-UHMUQfFqWYfmThMnQoZ1c... BUG=690915 Review-Url: https://codereview.chromium.org/2593063003 Cr-Commit-Position: refs/heads/master@{#455177} Committed: https://chromium.googlesource.com/chromium/src/+/321a96f376e57651a1ad987f5585... ==========
Message was sent while issue was closed.
Committed patchset #17 (id:620001) as https://chromium.googlesource.com/chromium/src/+/321a96f376e57651a1ad987f5585... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
