Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2915243002:
Fixing SSL error that occurs when underlying socket is blocked. (Closed)
|
Created:
3 years, 6 months ago by Taylor Brandstetter Modified:
3 years, 6 months ago CC:
webrtc-reviews_webrtc.org, tterriberry_mozilla.com, kwiberg-webrtc Target Ref:
refs/heads/master Project:
webrtc Visibility:
Public. |
DescriptionFixing SSL error that occurs when underlying socket is blocked.
BoringSSL (or OpenSSL) require that when SSL_write fails due to the
underlying socket being blocked, it's retried with the same parameters
until it succeeds. But we weren't doing this, and our socket
abstraction doesn't have an equivalent requirement. So when this was
occurring, we would just end up trying to send the next RTP or STUN
packet (instead of the packet that couldn't be sent), and BoringSSL
doesn't like that.
So, when this condition occurs now, we'll simply enter a "pending write"
mode and buffer the data that couldn't be completely sent. When the
underlying socket becomes writable again, or if Send is called again
before that happens, we retry sending the buffered data. Making both
BoringSSL and the upper layer of code that expects normal TCP socket
behavior happy.
Also adding some more logging, and fixing an issue with VirtualSocketServer
that made it behave slightly differently than PhysicalSocketServer when a
TCP packet is only partially read.
BUG=webrtc:7753
Review-Url: https://codereview.webrtc.org/2915243002
Cr-Commit-Position: refs/heads/master@{#18416}
Committed: https://chromium.googlesource.com/external/webrtc/+/ed3b986d63294d732cabe689e049b6e76d312980
Patch Set 1 #
Total comments: 30
Patch Set 2 : Responding to comments. #Patch Set 3 : Get rid of outdated comment. #Patch Set 4 : Send an additional message while socket is blocked. #
Total comments: 2
Messages
Total messages: 25 (11 generated)
deadbeef@webrtc.org changed reviewers: + pthatcher@webrtc.org
PTAL
lgtm, with suggestions Mostly just style stuff, but there is an extra test situation that I think we should check. If you feel it's urgent, submit without further approval. But I would like to see the comments addressed if possible. https://codereview.webrtc.org/2915243002/diff/1/webrtc/base/openssladapter.cc File webrtc/base/openssladapter.cc (left): https://codereview.webrtc.org/2915243002/diff/1/webrtc/base/openssladapter.cc... webrtc/base/openssladapter.cc:427: ssl_write_needs_read_ = false; Should this also clear pending_write_ and pending_data_? https://codereview.webrtc.org/2915243002/diff/1/webrtc/base/openssladapter.cc File webrtc/base/openssladapter.cc (right): https://codereview.webrtc.org/2915243002/diff/1/webrtc/base/openssladapter.cc... webrtc/base/openssladapter.cc:558: return -1; return SOCKET_ERROR or just: return ret; https://codereview.webrtc.org/2915243002/diff/1/webrtc/base/openssladapter.cc... webrtc/base/openssladapter.cc:559: } Might be more clear with an early return: if (pending_write_) { int ret = DoSslWrite(pending_data_.data(), pending_data_.size()); if (ret != static_cast<int>(pending_data_.size())) { // We couldn't finish sending the pending data, so we definitely can't // send any more data. Return with an EWOULDBLOCK error. SetError(EWOULDBLOCK); return -1; } // We completed sending the data previously passed into SSL_write! Now // we're allowed to send more data. pending_write_ = false; } Or, better yet, put this in a helper method: It seems like this should be in a helper method: bool MaybeRetrySslWrite() { if (!pending_write_) { return true; } if (DoSslWrite(pending_data_.data(), pending_data_.size()) != static_cast<int>(pending_data_.size())) { return false; } pending_write_ = true; return false; } Then, here just call: if (!MaybeRetrySslWrite()) { // We couldn't finish sending the pending data, so we definitely can't // send any more data. Return with an EWOULDBLOCK error. SetError(EWOULDBLOCK); return SOCKET_ERROR; } https://codereview.webrtc.org/2915243002/diff/1/webrtc/base/openssladapter.cc... webrtc/base/openssladapter.cc:611: return code; Should we check for SSL_ERROR_SSL here as well? https://codereview.webrtc.org/2915243002/diff/1/webrtc/base/openssladapter.cc... webrtc/base/openssladapter.cc:756: pending_write_ = false; Shouldn't we clear the pending_data_? https://codereview.webrtc.org/2915243002/diff/1/webrtc/base/openssladapter.cc... webrtc/base/openssladapter.cc:758: } With the helper method, this could just be: MaybeRetrySslWrite(); https://codereview.webrtc.org/2915243002/diff/1/webrtc/base/openssladapter.h File webrtc/base/openssladapter.h (right): https://codereview.webrtc.org/2915243002/diff/1/webrtc/base/openssladapter.h#... webrtc/base/openssladapter.h:96: Buffer pending_data_; Why do we need both? Can't the non-emptiness of pending_data_ be sufficient to know there is pending data to write? https://codereview.webrtc.org/2915243002/diff/1/webrtc/base/ssladapter_unitte... File webrtc/base/ssladapter_unittest.cc (right): https://codereview.webrtc.org/2915243002/diff/1/webrtc/base/ssladapter_unitte... webrtc/base/ssladapter_unittest.cc:418: ASSERT_EQ(-1, rv); Should we verify that sending the same thing while still blocked returns -1 and doesn't cause double buffering or something broken?
juberti@webrtc.org changed reviewers: + juberti@webrtc.org
https://codereview.webrtc.org/2915243002/diff/1/webrtc/base/openssladapter.cc File webrtc/base/openssladapter.cc (right): https://codereview.webrtc.org/2915243002/diff/1/webrtc/base/openssladapter.cc... webrtc/base/openssladapter.cc:343: SSL_MODE_ACCEPT_MOVING_WRITE_BUFFER); From the chat with davidben, I'm not sure we want to keep ENABLE_PARTIAL_WRITE. https://codereview.webrtc.org/2915243002/diff/1/webrtc/base/openssladapter.cc... webrtc/base/openssladapter.cc:460: uint32_t error_code; This feels like it should be factored into a function. https://codereview.webrtc.org/2915243002/diff/1/webrtc/base/openssladapter.cc... webrtc/base/openssladapter.cc:474: // Success! Maybe this case should come first? https://codereview.webrtc.org/2915243002/diff/1/webrtc/base/openssladapter.cc... webrtc/base/openssladapter.cc:509: if ((error == SSL_ERROR_WANT_READ || error == SSL_ERROR_WANT_WRITE) && It seems like this should be a low-level routine - it just takes what it gets and sends it. I would think the handling of pending_data_ would be done at the next level up - i.e., in OpenSSLAdapter::Send (similar to how pending_write_ is handled there). https://codereview.webrtc.org/2915243002/diff/1/webrtc/base/ssladapter_unitte... File webrtc/base/ssladapter_unittest.cc (right): https://codereview.webrtc.org/2915243002/diff/1/webrtc/base/ssladapter_unitte... webrtc/base/ssladapter_unittest.cc:218: // Read data received from the client and store it in our internal This shouldn't be necessary - we should trigger a new SE_READ even after a partial read. Otherwise, looping here could cause us to become corebound trying to read a busy stream. https://codereview.webrtc.org/2915243002/diff/1/webrtc/base/ssladapter_unitte... webrtc/base/ssladapter_unittest.cc:396: TEST_F(SSLAdapterTestTLS_RSA, TestTLSTransferWithBlockedSocket) { Put this into a TestTransferWithBlockedSocket function in the fixture class, perhaps?
Description was changed from ========== Fixing SSL error that occurs when underlying socket is blocked. BoringSSL (or OpenSSL) require that when SSL_write fails due to the underlying socket being blocked, it's retried with the same parameters until it succeeds. But we weren't doing this, and our socket abstraction doesn't have an equivalent requirement. So when this was occurring, we would just end up trying to send the next RTP or STUN packet (instead of the packet that couldn't be sent), and BoringSSL doesn't like that. So, when this condition occurs now, we'll simply enter a "pending write" mode and buffer the data that couldn't be completely sent. When the underlying socket becomes writable again, or if Send is called again before that happens, we retry sending the buffered data. Making both BoringSSL and the upper layer of code that expects normal TCP socket behavior happy. BUG=webrtc:7753 ========== to ========== Fixing SSL error that occurs when underlying socket is blocked. BoringSSL (or OpenSSL) require that when SSL_write fails due to the underlying socket being blocked, it's retried with the same parameters until it succeeds. But we weren't doing this, and our socket abstraction doesn't have an equivalent requirement. So when this was occurring, we would just end up trying to send the next RTP or STUN packet (instead of the packet that couldn't be sent), and BoringSSL doesn't like that. So, when this condition occurs now, we'll simply enter a "pending write" mode and buffer the data that couldn't be completely sent. When the underlying socket becomes writable again, or if Send is called again before that happens, we retry sending the buffered data. Making both BoringSSL and the upper layer of code that expects normal TCP socket behavior happy. Also adding some more logging, and fixing an issue with VirtualSocketServer that made it behave slightly differently than PhysicalSocketServer when a TCP packet is only partially read. BUG=webrtc:7753 ==========
Comments addressed. Probably worth another review. https://codereview.webrtc.org/2915243002/diff/1/webrtc/base/openssladapter.cc File webrtc/base/openssladapter.cc (left): https://codereview.webrtc.org/2915243002/diff/1/webrtc/base/openssladapter.cc... webrtc/base/openssladapter.cc:427: ssl_write_needs_read_ = false; On 2017/06/02 04:58:21, pthatcher1 wrote: > Should this also clear pending_write_ and pending_data_? I'll do it for consistency, but I don't see a way this class could be used after Close is called since it closes the underlying socket. https://codereview.webrtc.org/2915243002/diff/1/webrtc/base/openssladapter.cc File webrtc/base/openssladapter.cc (right): https://codereview.webrtc.org/2915243002/diff/1/webrtc/base/openssladapter.cc... webrtc/base/openssladapter.cc:343: SSL_MODE_ACCEPT_MOVING_WRITE_BUFFER); On 2017/06/02 05:35:44, juberti1 wrote: > From the chat with davidben, I'm not sure we want to keep ENABLE_PARTIAL_WRITE. Probably. But it's unrelated to this CL so I'll leave it as a TODO. https://codereview.webrtc.org/2915243002/diff/1/webrtc/base/openssladapter.cc... webrtc/base/openssladapter.cc:460: uint32_t error_code; On 2017/06/02 05:35:44, juberti1 wrote: > This feels like it should be factored into a function. Done. https://codereview.webrtc.org/2915243002/diff/1/webrtc/base/openssladapter.cc... webrtc/base/openssladapter.cc:474: // Success! On 2017/06/02 05:35:44, juberti1 wrote: > Maybe this case should come first? Done. https://codereview.webrtc.org/2915243002/diff/1/webrtc/base/openssladapter.cc... webrtc/base/openssladapter.cc:509: if ((error == SSL_ERROR_WANT_READ || error == SSL_ERROR_WANT_WRITE) && On 2017/06/02 05:35:43, juberti1 wrote: > It seems like this should be a low-level routine - it just takes what it gets > and sends it. I would think the handling of pending_data_ would be done at the > next level up - i.e., in OpenSSLAdapter::Send (similar to how pending_write_ is > handled there). Ok. Somehow the fact that the write failed due to "SSL_ERROR_WANT_READ" or "SSL_ERROR_WANT_WRITE" needs to be communicated though, so I'll make the error an out param I guess. https://codereview.webrtc.org/2915243002/diff/1/webrtc/base/openssladapter.cc... webrtc/base/openssladapter.cc:558: return -1; On 2017/06/02 04:58:21, pthatcher1 wrote: > return SOCKET_ERROR > > or just: > > return ret; Done. https://codereview.webrtc.org/2915243002/diff/1/webrtc/base/openssladapter.cc... webrtc/base/openssladapter.cc:559: } On 2017/06/02 04:58:21, pthatcher1 wrote: > Might be more clear with an early return: > > if (pending_write_) { > int ret = DoSslWrite(pending_data_.data(), pending_data_.size()); > if (ret != static_cast<int>(pending_data_.size())) { > // We couldn't finish sending the pending data, so we definitely can't > // send any more data. Return with an EWOULDBLOCK error. > SetError(EWOULDBLOCK); > return -1; > } > // We completed sending the data previously passed into SSL_write! Now > // we're allowed to send more data. > pending_write_ = false; > } > > > Or, better yet, put this in a helper method: > > > It seems like this should be in a helper method: > > bool MaybeRetrySslWrite() { > if (!pending_write_) { > return true; > } > if (DoSslWrite(pending_data_.data(), pending_data_.size()) != > static_cast<int>(pending_data_.size())) { > return false; > } > pending_write_ = true; > return false; > } > > > Then, here just call: > > if (!MaybeRetrySslWrite()) { > // We couldn't finish sending the pending data, so we definitely can't > // send any more data. Return with an EWOULDBLOCK error. > SetError(EWOULDBLOCK); > return SOCKET_ERROR; > } I don't like "MaybeRetrySslWrite" because it's not obvious what the return value means. I did the early return though. https://codereview.webrtc.org/2915243002/diff/1/webrtc/base/openssladapter.cc... webrtc/base/openssladapter.cc:611: return code; On 2017/06/02 04:58:21, pthatcher1 wrote: > Should we check for SSL_ERROR_SSL here as well? Done. https://codereview.webrtc.org/2915243002/diff/1/webrtc/base/openssladapter.cc... webrtc/base/openssladapter.cc:756: pending_write_ = false; On 2017/06/02 04:58:21, pthatcher1 wrote: > Shouldn't we clear the pending_data_? Would have been pointless before, but since I got rid of pending_write_ it's necessary. https://codereview.webrtc.org/2915243002/diff/1/webrtc/base/openssladapter.h File webrtc/base/openssladapter.h (right): https://codereview.webrtc.org/2915243002/diff/1/webrtc/base/openssladapter.h#... webrtc/base/openssladapter.h:96: Buffer pending_data_; On 2017/06/02 04:58:21, pthatcher1 wrote: > Why do we need both? Can't the non-emptiness of pending_data_ be sufficient to > know there is pending data to write? Sure, done. https://codereview.webrtc.org/2915243002/diff/1/webrtc/base/ssladapter_unitte... File webrtc/base/ssladapter_unittest.cc (right): https://codereview.webrtc.org/2915243002/diff/1/webrtc/base/ssladapter_unitte... webrtc/base/ssladapter_unittest.cc:218: // Read data received from the client and store it in our internal On 2017/06/02 05:35:44, juberti1 wrote: > This shouldn't be necessary - we should trigger a new SE_READ even after a > partial read. Otherwise, looping here could cause us to become corebound trying > to read a busy stream. Hmm. So is this an issue with VirtualSocketServer? Should it be queueing a task to call SignalReadEvent again if a partial read occurs? See updated patchset. https://codereview.webrtc.org/2915243002/diff/1/webrtc/base/ssladapter_unitte... webrtc/base/ssladapter_unittest.cc:396: TEST_F(SSLAdapterTestTLS_RSA, TestTLSTransferWithBlockedSocket) { On 2017/06/02 05:35:44, juberti1 wrote: > Put this into a TestTransferWithBlockedSocket function in the fixture class, > perhaps? I don't think that helps, since I only want to run this test with TLS sockets (and don't care about RSA vs. ECDSA). https://codereview.webrtc.org/2915243002/diff/1/webrtc/base/ssladapter_unitte... webrtc/base/ssladapter_unittest.cc:418: ASSERT_EQ(-1, rv); On 2017/06/02 04:58:22, pthatcher1 wrote: > Should we verify that sending the same thing while still blocked returns -1 and > doesn't cause double buffering or something broken? Done.
The CQ bit was checked by deadbeef@webrtc.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.webrtc.org/...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_asan on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/linux_asan/builds/25203)
lgtm again, with one small thing https://codereview.webrtc.org/2915243002/diff/1/webrtc/base/ssladapter_unitte... File webrtc/base/ssladapter_unittest.cc (right): https://codereview.webrtc.org/2915243002/diff/1/webrtc/base/ssladapter_unitte... webrtc/base/ssladapter_unittest.cc:418: ASSERT_EQ(-1, rv); On 2017/06/02 04:58:22, pthatcher1 wrote: > Should we verify that sending the same thing while still blocked returns -1 and > doesn't cause double buffering or something broken? I meant doing something like EXPECT_EQ(-1, client_->Send("Hello, world: 1023"));
Actually, I just realized a potential problem: If we return the size buffered while caching the packet, then AsyncTCPSocket::Send will fire SignalSentPacket immediately, which will then go into the BWE here: https://cs.chromium.org/chromium/src/third_party/webrtc/call/call.cc?type=cs&... Which means it will think it was sent much earlier than it was. If anything, I think this should cause the BWE to give a low estimate, which is perhaps good since there is back pressure and it should back off. But it's worth verifying that this is correct, and if it is leaving a comment in openssladapter.cc about it.
On 2017/06/02 14:06:08, pthatcher1 wrote: > Actually, I just realized a potential problem: > > If we return the size buffered while caching the packet, then > AsyncTCPSocket::Send will fire SignalSentPacket immediately, which will then go > into the BWE here: > https://cs.chromium.org/chromium/src/third_party/webrtc/call/call.cc?type=cs&... > > Which means it will think it was sent much earlier than it was. If anything, I > think this should cause the BWE to give a low estimate, which is perhaps good > since there is back pressure and it should back off. But it's worth verifying > that this is correct, and if it is leaving a comment in openssladapter.cc about > it. By the way, the only user of OpenSslAdapter is TurnPort, so we might be able to simplify our code structure. Perhaps just use And the only user of OpenSslStreamAdapter is DtlsTransportChannel, so we might be able to simplify there as well. And it means we don't need to fix this in OpenSslStreamAdapter, since it's never used with TLS. And the only SslAdapters are OpenSslAdapters, so we can simplify our code structure there as well. Perhaps just remove the split.
On 2017/06/02 14:06:08, pthatcher1 wrote: > Actually, I just realized a potential problem: > > If we return the size buffered while caching the packet, then > AsyncTCPSocket::Send will fire SignalSentPacket immediately, which will then go > into the BWE here: > https://cs.chromium.org/chromium/src/third_party/webrtc/call/call.cc?type=cs&... > > Which means it will think it was sent much earlier than it was. If anything, I > think this should cause the BWE to give a low estimate, which is perhaps good > since there is back pressure and it should back off. But it's worth verifying > that this is correct, and if it is leaving a comment in openssladapter.cc about > it. I don't see how this is different than adding a packet to the OS's buffer and not knowing when it will actually be sent. If we want the bandwidth estimator to react to delay introduced by filling local socket buffers, this seems like the right thing to do.
On 2017/06/02 14:10:49, pthatcher1 wrote: > > By the way, the only user of OpenSslAdapter is TurnPort, so we might be able to > simplify our code structure. Perhaps just use Use what? > And the only user of OpenSslStreamAdapter is DtlsTransportChannel, so we might > be able to simplify there as well. And it means we don't need to fix this in > OpenSslStreamAdapter, since it's never used with TLS. Not sure how this relates to this CL. > And the only SslAdapters are OpenSslAdapters, so we can simplify our code > structure there as well. Perhaps just remove the split. Sure, but that's separate from this CL.
https://codereview.webrtc.org/2915243002/diff/1/webrtc/base/ssladapter_unitte... File webrtc/base/ssladapter_unittest.cc (right): https://codereview.webrtc.org/2915243002/diff/1/webrtc/base/ssladapter_unitte... webrtc/base/ssladapter_unittest.cc:418: ASSERT_EQ(-1, rv); On 2017/06/02 13:24:24, pthatcher1 wrote: > On 2017/06/02 04:58:22, pthatcher1 wrote: > > Should we verify that sending the same thing while still blocked returns -1 > and > > doesn't cause double buffering or something broken? > > I meant doing something like > > > EXPECT_EQ(-1, client_->Send("Hello, world: 1023")); This already tests that the last message (for which -1 was returned) isn't sent. Do you just want it to test 2 unsent messages instead of 1? If so, done.
The CQ bit was checked by deadbeef@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from pthatcher@webrtc.org Link to the patchset: https://codereview.webrtc.org/2915243002/#ps60001 (title: "Send an additional message while socket is blocked.")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
CQ is committing da patch.
Bot data: {"patchset_id": 60001, "attempt_start_ts": 1496423459600990,
"parent_rev": "25fd62bcc00dd4dbf725963ca861d9c70e7b2170", "commit_rev":
"ed3b986d63294d732cabe689e049b6e76d312980"}
Message was sent while issue was closed.
Description was changed from ========== Fixing SSL error that occurs when underlying socket is blocked. BoringSSL (or OpenSSL) require that when SSL_write fails due to the underlying socket being blocked, it's retried with the same parameters until it succeeds. But we weren't doing this, and our socket abstraction doesn't have an equivalent requirement. So when this was occurring, we would just end up trying to send the next RTP or STUN packet (instead of the packet that couldn't be sent), and BoringSSL doesn't like that. So, when this condition occurs now, we'll simply enter a "pending write" mode and buffer the data that couldn't be completely sent. When the underlying socket becomes writable again, or if Send is called again before that happens, we retry sending the buffered data. Making both BoringSSL and the upper layer of code that expects normal TCP socket behavior happy. Also adding some more logging, and fixing an issue with VirtualSocketServer that made it behave slightly differently than PhysicalSocketServer when a TCP packet is only partially read. BUG=webrtc:7753 ========== to ========== Fixing SSL error that occurs when underlying socket is blocked. BoringSSL (or OpenSSL) require that when SSL_write fails due to the underlying socket being blocked, it's retried with the same parameters until it succeeds. But we weren't doing this, and our socket abstraction doesn't have an equivalent requirement. So when this was occurring, we would just end up trying to send the next RTP or STUN packet (instead of the packet that couldn't be sent), and BoringSSL doesn't like that. So, when this condition occurs now, we'll simply enter a "pending write" mode and buffer the data that couldn't be completely sent. When the underlying socket becomes writable again, or if Send is called again before that happens, we retry sending the buffered data. Making both BoringSSL and the upper layer of code that expects normal TCP socket behavior happy. Also adding some more logging, and fixing an issue with VirtualSocketServer that made it behave slightly differently than PhysicalSocketServer when a TCP packet is only partially read. BUG=webrtc:7753 Review-Url: https://codereview.webrtc.org/2915243002 Cr-Commit-Position: refs/heads/master@{#18416} Committed: https://chromium.googlesource.com/external/webrtc/+/ed3b986d63294d732cabe689e... ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001) as https://chromium.googlesource.com/external/webrtc/+/ed3b986d63294d732cabe689e...
Message was sent while issue was closed.
lgtm with nits https://codereview.webrtc.org/2915243002/diff/1/webrtc/base/ssladapter_unitte... File webrtc/base/ssladapter_unittest.cc (right): https://codereview.webrtc.org/2915243002/diff/1/webrtc/base/ssladapter_unitte... webrtc/base/ssladapter_unittest.cc:396: TEST_F(SSLAdapterTestTLS_RSA, TestTLSTransferWithBlockedSocket) { On 2017/06/02 06:49:14, Taylor Brandstetter wrote: > On 2017/06/02 05:35:44, juberti1 wrote: > > Put this into a TestTransferWithBlockedSocket function in the fixture class, > > perhaps? > > I don't think that helps, since I only want to run this test with TLS sockets > (and don't care about RSA vs. ECDSA). Sure, but I was thinking it would be easiest for future maintainers to have all the test code that does real work arranged similarly. https://codereview.webrtc.org/2915243002/diff/60001/webrtc/base/openssladapte... File webrtc/base/openssladapter.cc (right): https://codereview.webrtc.org/2915243002/diff/60001/webrtc/base/openssladapte... webrtc/base/openssladapter.cc:569: pending_data_.empty()) { Is it possible for pending_data_ to *not* be empty here?
Message was sent while issue was closed.
https://codereview.webrtc.org/2915243002/diff/60001/webrtc/base/openssladapte... File webrtc/base/openssladapter.cc (right): https://codereview.webrtc.org/2915243002/diff/60001/webrtc/base/openssladapte... webrtc/base/openssladapter.cc:569: pending_data_.empty()) { On 2017/06/02 17:47:22, juberti1 wrote: > Is it possible for pending_data_ to *not* be empty here? Oh, it isn't possible now that this code was shuffled around. I'll make a follow-up CL that adds a DCHECK. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
