Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2736103002:
Only pump SSL_read extra times when there is data available synchronously. (Closed)
DescriptionOnly pump SSL_read extra times when there is data available synchronously.
If SSL_read is run an extra time, it will implicitly trigger a read from
the transport. However, if we've already returned data, it's possible
the consumer had no need for this extra data to begin with. The end
result is that, in this situation, we would leave buffers allocated and
sockets in the select loop when we shouldn't.
Prior to the SocketBIOAdapter change, we wouldn't read from the
transport as that would be nothing driving the DoTransportIO loop
(though if writes were driving the transport we would read, which was
weird). This restores the first behavior.
BUG=652456
Review-Url: https://codereview.chromium.org/2736103002
Cr-Commit-Position: refs/heads/master@{#455297}
Committed: https://chromium.googlesource.com/chromium/src/+/8ea6b17294f2ce1ea405e020ca07d2e048a6a083
Patch Set 1 #
Total comments: 5
Patch Set 2 : rebase #Patch Set 3 : svaldez comments, SSLClientSocketReadTest #
Messages
Total messages: 22 (16 generated)
The CQ bit was checked by davidben@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...
davidben@chromium.org changed reviewers: + svaldez@chromium.org
As is typical, this CL is almost entirely tests. This would normally be a good thing, except the CL is also almost entirely testing boilerplate and not meaningful test.
+xunjieli FYI
Description was changed from ========== Only pump SSL_read extra times when there is data synchronously. If SSL_read is run an extra time, it will implicitly trigger a read from the transport. However, if we've already returned data, it's possible the consumer had no need for this extra data to begin with. The end result is that, in this situation, we would leave buffers allocated and sockets in the select loop when we shouldn't. Prior to the SocketBIOAdapter change, we wouldn't read from the transport as that would be nothing driving the DoTransportIO loop (though if writes were driving the transport we would read, which was weird). This restores that behavior. BUG=652456 ========== to ========== Only pump SSL_read extra times when there is data available synchronously. If SSL_read is run an extra time, it will implicitly trigger a read from the transport. However, if we've already returned data, it's possible the consumer had no need for this extra data to begin with. The end result is that, in this situation, we would leave buffers allocated and sockets in the select loop when we shouldn't. Prior to the SocketBIOAdapter change, we wouldn't read from the transport as that would be nothing driving the DoTransportIO loop (though if writes were driving the transport we would read, which was weird). This restores that behavior. BUG=652456 ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromeos_amd64-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...) chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...) chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...) linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) 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...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
lgtm https://codereview.chromium.org/2736103002/diff/1/net/socket/ssl_client_socke... File net/socket/ssl_client_socket_impl.cc (right): https://codereview.chromium.org/2736103002/diff/1/net/socket/ssl_client_socke... net/socket/ssl_client_socket_impl.cc:1450: // Continue processing records as long as there is more data available Move this comment before the do-while loop. https://codereview.chromium.org/2736103002/diff/1/net/socket/ssl_client_socke... File net/socket/ssl_client_socket_unittest.cc (right): https://codereview.chromium.org/2736103002/diff/1/net/socket/ssl_client_socke... net/socket/ssl_client_socket_unittest.cc:3642: ASSERT_THAT(server_listener.Listen(IPEndPoint(lo_address, 0), 1), IsOk()); Any reason not just to inline lo_address (not used anywhere else). https://codereview.chromium.org/2736103002/diff/1/net/socket/ssl_client_socke... net/socket/ssl_client_socket_unittest.cc:3677: HostPortPair("localhost", server_address.port()), SSLConfig())); HostPortPair::FromIPEndpoint(server_address)
The CQ bit was checked by davidben@chromium.org to run a CQ dry run
https://codereview.chromium.org/2736103002/diff/1/net/socket/ssl_client_socke... File net/socket/ssl_client_socket_unittest.cc (right): https://codereview.chromium.org/2736103002/diff/1/net/socket/ssl_client_socke... net/socket/ssl_client_socket_unittest.cc:3642: ASSERT_THAT(server_listener.Listen(IPEndPoint(lo_address, 0), 1), IsOk()); On 2017/03/07 22:05:54, svaldez wrote: > Any reason not just to inline lo_address (not used anywhere else). Done. https://codereview.chromium.org/2736103002/diff/1/net/socket/ssl_client_socke... net/socket/ssl_client_socket_unittest.cc:3677: HostPortPair("localhost", server_address.port()), SSLConfig())); On 2017/03/07 22:05:54, svaldez wrote: > HostPortPair::FromIPEndpoint(server_address) Done.
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 davidben@chromium.org
The CQ bit was checked by davidben@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from svaldez@chromium.org Link to the patchset: https://codereview.chromium.org/2736103002/#ps40001 (title: "svaldez comments, SSLClientSocketReadTest")
The CQ bit was unchecked by davidben@chromium.org
Description was changed from ========== Only pump SSL_read extra times when there is data available synchronously. If SSL_read is run an extra time, it will implicitly trigger a read from the transport. However, if we've already returned data, it's possible the consumer had no need for this extra data to begin with. The end result is that, in this situation, we would leave buffers allocated and sockets in the select loop when we shouldn't. Prior to the SocketBIOAdapter change, we wouldn't read from the transport as that would be nothing driving the DoTransportIO loop (though if writes were driving the transport we would read, which was weird). This restores that behavior. BUG=652456 ========== to ========== Only pump SSL_read extra times when there is data available synchronously. If SSL_read is run an extra time, it will implicitly trigger a read from the transport. However, if we've already returned data, it's possible the consumer had no need for this extra data to begin with. The end result is that, in this situation, we would leave buffers allocated and sockets in the select loop when we shouldn't. Prior to the SocketBIOAdapter change, we wouldn't read from the transport as that would be nothing driving the DoTransportIO loop (though if writes were driving the transport we would read, which was weird). This restores the first behavior. BUG=652456 ==========
The CQ bit was checked by davidben@chromium.org
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": 40001, "attempt_start_ts": 1488927753915710,
"parent_rev": "5b96afbdfc2b9129c318eb62997d9e95e4069e94", "commit_rev":
"8ea6b17294f2ce1ea405e020ca07d2e048a6a083"}
Message was sent while issue was closed.
Description was changed from ========== Only pump SSL_read extra times when there is data available synchronously. If SSL_read is run an extra time, it will implicitly trigger a read from the transport. However, if we've already returned data, it's possible the consumer had no need for this extra data to begin with. The end result is that, in this situation, we would leave buffers allocated and sockets in the select loop when we shouldn't. Prior to the SocketBIOAdapter change, we wouldn't read from the transport as that would be nothing driving the DoTransportIO loop (though if writes were driving the transport we would read, which was weird). This restores the first behavior. BUG=652456 ========== to ========== Only pump SSL_read extra times when there is data available synchronously. If SSL_read is run an extra time, it will implicitly trigger a read from the transport. However, if we've already returned data, it's possible the consumer had no need for this extra data to begin with. The end result is that, in this situation, we would leave buffers allocated and sockets in the select loop when we shouldn't. Prior to the SocketBIOAdapter change, we wouldn't read from the transport as that would be nothing driving the DoTransportIO loop (though if writes were driving the transport we would read, which was weird). This restores the first behavior. BUG=652456 Review-Url: https://codereview.chromium.org/2736103002 Cr-Commit-Position: refs/heads/master@{#455297} Committed: https://chromium.googlesource.com/chromium/src/+/8ea6b17294f2ce1ea405e020ca07... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as https://chromium.googlesource.com/chromium/src/+/8ea6b17294f2ce1ea405e020ca07... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
