New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 859674 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: Jul 10
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 3
Type: Bug



Sign in to add a comment

QuicChromiumPacketWriter::write_blocked_ is not set correctly with connection migration

Project Member Reported by zhongyi@chromium.org, Jul 2

Issue description

QuicChromiumPacketWriter currently keeps a boolean |write_blocked_| to indicate whether a write is currently in flight. 

It is set to true when
a) Socket::Write() returns ERR_IO_PENDING indicate the previous packet write is asynchronous. ref: QuicChromiumPacketWriter::WritePacketToSocketImpl()
b) A retry after write error is in progress. ref: QuicChromiumPacketWriter::MaybeRetryAfterWriteError()
c) QuicChromiumPacketWrite::set_write_blocked(true) is invoked
c.1) when connection migration has no new network connected ref: QuicChromiumClientSession::OnNoNewNetwork()
c.2) connection migration is wrapping up and migrates to an alternate socket. ref:QuicChromiumClientSession::MigrateToSocket()

It is set to false when 
i) an asynchronous write is completed. ref: QuicChromiumPacketWriter::OnWriteComplete
ii) QuicChromiumPacketWriter::set_write_blocked(false) is invoked when 
connection migration manages to migrate to the alternate socket and is about to write to the "new" socket.
iii) QuicChromiumPacketWriter::SetWritable() is invoked, which is mostly used in non-chromium code.

a) and b) indicate that the write is blocked on the socket level. 
c.1 and c.2 indicate that the write is blocked on the network level.
i) indicates that write is unblocked on the socket level
ii) indicates that write is unblocked at the network level.

There would be no issue if connection migration is not turned on since network level block is introduced in connection migration.
Current code doesn't differentiate the two different causes, which will cause a write being unblocked incorrectly, in particular, we want to unblock the network level by the time calling set_write_blocked(false), while the write was blocked on the socket level previously. 

A more concrete example is laid as follows: 
connection migration is on
- connection detects path degrading, or session attempts to migrate back to the default network
- a new socket is created to probe the alternate network
- first connectivity probe is sent
... alarm fires....
- second connectivity probe is sent, asynchronous write returns ERR_IO_PENDING, set the probing write blocked (at socket level)
- response to first connectivity probe is received
- connection migration proceeds by MigrateToSocket, set the writer blocked (at network level)
- connection migration wraps up by WriteToNewSocket, set the writer unblocked (at network level)
- A new write comes in, found the writer unblocked, may attempt to write to the blocked writer, which blows up. 
 
Project Member

Comment 1 by bugdroid1@chromium.org, Jul 10

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/b3bc982c22ec4aa70a1a5e2ee767304fb0d7a098

commit b3bc982c22ec4aa70a1a5e2ee767304fb0d7a098
Author: Zhongyi Shi <zhongyi@chromium.org>
Date: Tue Jul 10 19:59:24 2018

Connection Migration V2: split the QuicChromiumPacketWriter's write blocked status into two separate invariants.

- |write_in_progress_| is tracking writer status, and is set when an asynchronous write doesn't complete, or HandleWriteError/MaybeRetryAfterWriteError is in progress.

- |force_write_blocked_| is tracking whether writer should be forced blocked regardless of |write_in_progress_|.

QuicChromiumClientSession will set force_write_blocked to true when no alternate network is available or connection migration is in progress to complete to ensure no packets can be written until migration completes. QuicChromiumClientSession::WriteToNewPacket will undo force writer blocked. If packet writer becomes unblocked immediately or after the in-flight write completes, QuicChromiumClientSession::OnWriteUnblocked will be invoked to send a packet after migration. The packet can be a cached packet if |packet_| is set. If there's no cached packet, a queued packet will be sent if there's any, and a ping packet will be sent if writer is still unblocked.

See  http://crbug.com/859674  for detailed motivations.

Bug:  859674 
Change-Id: Icdbbb5e14f8439c6601e2c19d9a03f05f17d4bc0
Reviewed-on: https://chromium-review.googlesource.com/1125286
Commit-Queue: Zhongyi Shi <zhongyi@chromium.org>
Reviewed-by: Ryan Hamilton <rch@chromium.org>
Cr-Commit-Position: refs/heads/master@{#573869}
[modify] https://crrev.com/b3bc982c22ec4aa70a1a5e2ee767304fb0d7a098/net/quic/chromium/quic_chromium_client_session.cc
[modify] https://crrev.com/b3bc982c22ec4aa70a1a5e2ee767304fb0d7a098/net/quic/chromium/quic_chromium_client_session.h
[modify] https://crrev.com/b3bc982c22ec4aa70a1a5e2ee767304fb0d7a098/net/quic/chromium/quic_chromium_packet_writer.cc
[modify] https://crrev.com/b3bc982c22ec4aa70a1a5e2ee767304fb0d7a098/net/quic/chromium/quic_chromium_packet_writer.h
[modify] https://crrev.com/b3bc982c22ec4aa70a1a5e2ee767304fb0d7a098/net/quic/chromium/quic_stream_factory_test.cc

Status: Fixed (was: Assigned)
Fix in #1 landed in 69.0.3488.0. 

Sign in to add a comment