Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2319343004:
Makes migration on write error asynchronous to avoid reentrancy issues (Closed)
|
Created:
4 years, 3 months ago by Jana Modified:
4 years, 3 months ago Reviewers:
Ryan Hamilton CC:
chromium-reviews, cbentzel+watch_chromium.org, Jo Kulik, Fan Yang Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionThis CL refactors the code for migration on write error, making it asynchronous to avoid reentrancyissues. Also simplifies some methods in QuicStreamFactory.
BUG=646145
Committed: https://crrev.com/9f3037153a2feb63bbaeaf07ab35c8ff9a3096ad
Cr-Commit-Position: refs/heads/master@{#418128}
Patch Set 1 #Patch Set 2 : Async tests added. #Patch Set 3 : More betterment and testing. #Patch Set 4 : Cleaning up. #
Total comments: 27
Patch Set 5 : responses to comments #
Total comments: 6
Patch Set 6 : Avoids multiple tasks from repeating packet write. #
Total comments: 2
Patch Set 7 : Addesses comments. #Patch Set 8 : added tests for async write before notification and fixed call to OnCanWrite #
Total comments: 36
Patch Set 9 : rebased #Patch Set 10 : Addressed comments and removed use of write_pending_. #Patch Set 11 : Do not try to migrate on write error if migration is disabled. #Patch Set 12 : fixed type #Patch Set 13 : Fixed typo. #
Total comments: 4
Patch Set 14 : comments addressed. #Patch Set 15 : synced and rebased #Dependent Patchsets: Messages
Total messages: 36 (11 generated)
Description was changed from ========== Makes the code path for migration on write error asynchronous to avoid re-entrance issues. BUG= ========== to ========== This CL refactors the code for migration on write error, making it asynchronous to avoid re-entrance issues. Also simplifies some methods in the QuicStreamFactory. BUG= ==========
Description was changed from ========== This CL refactors the code for migration on write error, making it asynchronous to avoid re-entrance issues. Also simplifies some methods in the QuicStreamFactory. BUG= ========== to ========== This CL refactors the code for migration on write error, making it asynchronous to avoid re-entrance issues. Also simplifies some methods in QuicStreamFactory. BUG= ==========
jri@chromium.org changed reviewers: + rch@chromium.org
This CL does not yet include a pause period between migration trigger and completion -- I'm planning to do that in a separate CL.
Have to go run some errands, but here are some partial comments. I think there's some subtlety about the write-blocked-ness of the new writer. https://codereview.chromium.org/2319343004/diff/60001/net/quic/chromium/quic_... File net/quic/chromium/quic_chromium_client_session.cc (right): https://codereview.chromium.org/2319343004/diff/60001/net/quic/chromium/quic_... net/quic/chromium/quic_chromium_client_session.cc:240: packet_(nullptr), nit: no need for this as the default constructor will do this. https://codereview.chromium.org/2319343004/diff/60001/net/quic/chromium/quic_... net/quic/chromium/quic_chromium_client_session.cc:969: // session tries migrating to a new writer/socket. Since the actual write happen after yet another PostTask, this comment is a bit inaccurate. I think it might be more clear to say something like: // Cause the packet writer to return ERR_IO_PENDING and block so // that the actual migration happens from the message loop instead // of under the call stack of QuicConnection::WritePacket. (or some such) https://codereview.chromium.org/2319343004/diff/60001/net/quic/chromium/quic_... net/quic/chromium/quic_chromium_client_session.cc:976: // different migration attempt. Do not attempt another migration. nit: As written, the body of this if is 3 lines lone, which requires {}s. But you can move the comments to above the if, and it'll still read cleanly and then you can avoid the {}s. https://codereview.chromium.org/2319343004/diff/60001/net/quic/chromium/quic_... net/quic/chromium/quic_chromium_client_session.cc:996: } Under what circumstances is this code path hit? Can it happen from write error? If so, as far as the connection is concerned, there is a write outstanding and the writer is write blocked. (Or at least it was when the original write error occurred). At this point, the connection is hanging around waiting for OnCanWrite to resume the write path. By calling SendPing here, we're actually causing a write in a place where the connection isn't expecting it. It might all work correctly, but it feels strange. Perhaps this should do connection->OnCanWrite() before sending the ping? https://codereview.chromium.org/2319343004/diff/60001/net/quic/chromium/quic_... net/quic/chromium/quic_chromium_client_session.cc:997: WriteResult result = Let's add a comment here to explain that the connection is waiting for the original write to complete asynchronously and so this code has to propagate any synchronous write results back to the connection. https://codereview.chromium.org/2319343004/diff/60001/net/quic/chromium/quic_... net/quic/chromium/quic_chromium_client_session.cc:1004: connection()->OnWriteError(result.error_code); In theory, we should never get a WriteError because HandleWriteError will make that to WRITE_BLOCKED, right? https://codereview.chromium.org/2319343004/diff/60001/net/quic/chromium/quic_... net/quic/chromium/quic_chromium_client_session.cc:1008: connection()->OnCanWrite(); nit: how about if (result.error_code == ERR_IO_PENDING) return; if (result.error_code < 0) connection()->OnWriteError(result.error_code); else connection()->OnCanWrite(); Not any fewer lines, but it avoid ERR_IO_PENDING being checked twice. Alternatively, it might make sense to move this logic into the writer. Perhaps we should have a helper method something like: ResendPacketAfterMigration(packet) which calls WritePacketToSocket and then knows that it has to propogate sync write results to the connection? *shurg* Either way. https://codereview.chromium.org/2319343004/diff/60001/net/quic/chromium/quic_... net/quic/chromium/quic_chromium_client_session.cc:1023: stream_factory_->MaybeMigrateSingleSession(this, EARLY_MIGRATION); Is it possible that this is called while a write is outstanding? If so, this means that WriteToNewSocket can get called when there is no |packet| but while the connection is waiting for a write to complete (and my earlier comment about needing OnCanWrite() apply) We don't yet have the "pause when no new network" code, right? When we do, will we need (or want) to mark the writer write blocked? https://codereview.chromium.org/2319343004/diff/60001/net/quic/chromium/quic_... net/quic/chromium/quic_chromium_client_session.cc:1246: weak_factory_.GetWeakPtr(), packet_)); Since we're not writing immediately, I think we need to make sure the new writer is write blocked so that nothing else tries to write before we're get around to resending |packet_| otherwise we might call WritePacketToNewSocket() while a write is outstanding. https://codereview.chromium.org/2319343004/diff/60001/net/quic/chromium/quic_... net/quic/chromium/quic_chromium_client_session.cc:1251: packet_ = nullptr; Is (i) actually correct? If I understand correctly, packet_ won't need to be reused until it's been written, right? (So it could be cleared in WriteToNewSocket).
No need to do this, but it looks like you could land the QuicStreamFactory changes in a stand-alone small CL, if you wish? (or maybe there's a subtlety I missing. https://codereview.chromium.org/2319343004/diff/60001/net/quic/chromium/mock_... File net/quic/chromium/mock_network_change_notifier.h (right): https://codereview.chromium.org/2319343004/diff/60001/net/quic/chromium/mock_... net/quic/chromium/mock_network_change_notifier.h:30: void QueueNetworkMadeDefault(NetworkChangeNotifier::NetworkHandle network); Can you add some comments. It's not obvious how this is different from the previous method. https://codereview.chromium.org/2319343004/diff/60001/net/quic/chromium/quic_... File net/quic/chromium/quic_stream_factory.cc (right): https://codereview.chromium.org/2319343004/diff/60001/net/quic/chromium/quic_... net/quic/chromium/quic_stream_factory.cc:1577: return MigrateSessionToNewNetwork(session, new_network, Seems like it might just be cleaner to have this method (and Inner) just return a MigrationResult. https://codereview.chromium.org/2319343004/diff/60001/net/quic/chromium/quic_... net/quic/chromium/quic_stream_factory.cc:1646: // migration, making subsequent migration impossible. This is now reachable because of migration on write error, right? Also, if a session is going away, during a network change it seems like we should still migrate it. What do you think? (not necessarily in this CL, but something to think about)
Thanks, Ryan. Comments addressed. https://codereview.chromium.org/2319343004/diff/60001/net/quic/chromium/mock_... File net/quic/chromium/mock_network_change_notifier.h (right): https://codereview.chromium.org/2319343004/diff/60001/net/quic/chromium/mock_... net/quic/chromium/mock_network_change_notifier.h:30: void QueueNetworkMadeDefault(NetworkChangeNotifier::NetworkHandle network); On 2016/09/10 16:40:34, Ryan Hamilton wrote: > Can you add some comments. It's not obvious how this is different from the > previous method. Done. https://codereview.chromium.org/2319343004/diff/60001/net/quic/chromium/quic_... File net/quic/chromium/quic_chromium_client_session.cc (right): https://codereview.chromium.org/2319343004/diff/60001/net/quic/chromium/quic_... net/quic/chromium/quic_chromium_client_session.cc:240: packet_(nullptr), On 2016/09/10 16:29:47, Ryan Hamilton wrote: > nit: no need for this as the default constructor will do this. Done. https://codereview.chromium.org/2319343004/diff/60001/net/quic/chromium/quic_... net/quic/chromium/quic_chromium_client_session.cc:969: // session tries migrating to a new writer/socket. On 2016/09/10 16:29:46, Ryan Hamilton wrote: > Since the actual write happen after yet another PostTask, this comment is a bit > inaccurate. I think it might be more clear to say something like: > > // Cause the packet writer to return ERR_IO_PENDING and block so > // that the actual migration happens from the message loop instead > // of under the call stack of QuicConnection::WritePacket. > > (or some such) Much more accurate -- done. https://codereview.chromium.org/2319343004/diff/60001/net/quic/chromium/quic_... net/quic/chromium/quic_chromium_client_session.cc:976: // different migration attempt. Do not attempt another migration. On 2016/09/10 16:29:46, Ryan Hamilton wrote: > nit: As written, the body of this if is 3 lines lone, which requires {}s. But > you can move the comments to above the if, and it'll still read cleanly and then > you can avoid the {}s. I assumed that comments didn't count. It's about readability though so makes sense that comments would. Fixed. https://codereview.chromium.org/2319343004/diff/60001/net/quic/chromium/quic_... net/quic/chromium/quic_chromium_client_session.cc:996: } On 2016/09/10 16:29:46, Ryan Hamilton wrote: > Under what circumstances is this code path hit? Can it happen from write error? > If so, as far as the connection is concerned, there is a write outstanding and > the writer is write blocked. (Or at least it was when the original write error > occurred). At this point, the connection is hanging around waiting for > OnCanWrite to resume the write path. By calling SendPing here, we're actually > causing a write in a place where the connection isn't expecting it. It might all > work correctly, but it feels strange. Perhaps this should do > connection->OnCanWrite() before sending the ping? You're partially right that you can hit this code via the write error path, but there's an important subtlety. This method can be called via a write error or via NCN events. If there's only an NCN event that leads here, packet must be null and the connection is not write blocked. If there's only a write error that leads here, then packet must be non-null, since HandleWriteError stores a packet in the packet_ member. The packet_ member is cleared in two cases: (i) after we post a task to call this method with the packet, or (ii) num_migrations exceeded (see MigrateToSocket below.) If the WriteError event and an NCN event are interleaved, leading to two WriteToNewSockets being posted and executed, it doesn't matter which WriteToNewSocket occurs first. The first one will rewrite packet_ and unblock the connection (or close on error), and the second one will send a PING packet. (The PING is not necessary, and I think I can tighten this -- I'll take a shot. But this only happens under a very specific race -- NCN event posts a write task after MigrateSessionOnWriteError completes but before WriteToNewSocket executes -- and the cost is that an unnecessary PING packet is sent.) Make sense? Ok, coming back to this after some mode thinking -- the code didn't quite do what I said it did, but I've fixed it now to do precisely what I describe above. The problem was that MigrateToSocket was posting a WriteToNewSocket with a packet parameter. Now, if WriteToNewSocket was posted by an NCN event first (with packet=nullptr), and then a write error occurred, causing the writer to be blocked before the NCN-event-posted WriteToNewSocket is executed, we could end up in the situation you describe. This is hard to arrange in the test because I need to carefully crank the message loop to arrange for these events to happen in the order I described. (I'm trying to write tests with all possible interleavings... but I think I'll need to use the test runner and do this by hand.) I've changed the code to not post any packet parameter with WriteToNewSocket, but for the method to decide on execution which packet to write. Meaning that the packet_ member is populated at the beginning of the WriteError process, and set to null at the end of it, in WriteToNewSocket. If an NCN-triggered WriteToNewSocket shows up during this process, it will do the right thing of writing packet_ instead of sending a ping. https://codereview.chromium.org/2319343004/diff/60001/net/quic/chromium/quic_... net/quic/chromium/quic_chromium_client_session.cc:997: WriteResult result = On 2016/09/10 16:29:46, Ryan Hamilton wrote: > Let's add a comment here to explain that the connection is waiting for the > original write to complete asynchronously and so this code has to propagate any > synchronous write results back to the connection. Done. https://codereview.chromium.org/2319343004/diff/60001/net/quic/chromium/quic_... net/quic/chromium/quic_chromium_client_session.cc:1004: connection()->OnWriteError(result.error_code); On 2016/09/10 16:29:47, Ryan Hamilton wrote: > In theory, we should never get a WriteError because HandleWriteError will make > that to WRITE_BLOCKED, right? Correct. I see this in the tests on multiple write errors. I've turned it into a DCHECK instead. https://codereview.chromium.org/2319343004/diff/60001/net/quic/chromium/quic_... net/quic/chromium/quic_chromium_client_session.cc:1008: connection()->OnCanWrite(); On 2016/09/10 16:29:47, Ryan Hamilton wrote: > nit: how about > > if (result.error_code == ERR_IO_PENDING) > return; > > if (result.error_code < 0) > connection()->OnWriteError(result.error_code); > else > connection()->OnCanWrite(); > > Not any fewer lines, but it avoid ERR_IO_PENDING being checked twice. > > Alternatively, it might make sense to move this logic into the writer. Perhaps > we should have a helper method something like: > > ResendPacketAfterMigration(packet) > > which calls WritePacketToSocket and then knows that it has to propogate sync > write results to the connection? > > *shurg* Either way. > Done. I have a mild preference for it to stay here, since this method isn't particularly long and I think it's more readable this way. With the DCHECK, I think it belongs here. https://codereview.chromium.org/2319343004/diff/60001/net/quic/chromium/quic_... net/quic/chromium/quic_chromium_client_session.cc:1023: stream_factory_->MaybeMigrateSingleSession(this, EARLY_MIGRATION); On 2016/09/10 16:29:47, Ryan Hamilton wrote: > Is it possible that this is called while a write is outstanding? If so, this > means that WriteToNewSocket can get called when there is no |packet| but while > the connection is waiting for a write to complete (and my earlier comment about > needing OnCanWrite() apply) > > We don't yet have the "pause when no new network" code, right? When we do, will > we need (or want) to mark the writer write blocked? This should be handled in the same way as NCN-migration now. There are interactions between early and other migrations, but I'm leaving early migration testing out until after this CL. https://codereview.chromium.org/2319343004/diff/60001/net/quic/chromium/quic_... net/quic/chromium/quic_chromium_client_session.cc:1246: weak_factory_.GetWeakPtr(), packet_)); On 2016/09/10 16:29:46, Ryan Hamilton wrote: > Since we're not writing immediately, I think we need to make sure the new writer > is write blocked so that nothing else tries to write before we're get around to > resending |packet_| otherwise we might call WritePacketToNewSocket() while a > write is outstanding. Good point. I'll add a ShouldWriteBlock to the delegate. Done. https://codereview.chromium.org/2319343004/diff/60001/net/quic/chromium/quic_... net/quic/chromium/quic_chromium_client_session.cc:1251: packet_ = nullptr; On 2016/09/10 16:29:47, Ryan Hamilton wrote: > Is (i) actually correct? If I understand correctly, packet_ won't need to be > reused until it's been written, right? (So it could be cleared in > WriteToNewSocket). Right, I think the comment wasn't clear -- but I've moved all this to in WriteToNewSocket now. https://codereview.chromium.org/2319343004/diff/60001/net/quic/chromium/quic_... File net/quic/chromium/quic_stream_factory.cc (right): https://codereview.chromium.org/2319343004/diff/60001/net/quic/chromium/quic_... net/quic/chromium/quic_stream_factory.cc:1577: return MigrateSessionToNewNetwork(session, new_network, On 2016/09/10 16:40:34, Ryan Hamilton wrote: > Seems like it might just be cleaner to have this method (and Inner) just return > a MigrationResult. Done. https://codereview.chromium.org/2319343004/diff/60001/net/quic/chromium/quic_... net/quic/chromium/quic_stream_factory.cc:1646: // migration, making subsequent migration impossible. On 2016/09/10 16:40:34, Ryan Hamilton wrote: > This is now reachable because of migration on write error, right? Yes -- comment removed. I had thought at first that going away prevented a session from migrating, but I don't thin it's the case. > Also, if a session is going away, during a network change it seems like we > should still migrate it. What do you think? (not necessarily in this CL, but > something to think about) Yes, I think so too, and it should already happen. Marking a session as going away does not keep it from migrating, AFAICT.
Oh -- and I did think about it, but it's tricky to just do the stream factory changes. Esp. since the passing around of packet is tied to changes in session.
I've cleaned up the code and (I think) made it more readable. https://codereview.chromium.org/2319343004/diff/60001/net/quic/chromium/quic_... File net/quic/chromium/quic_chromium_client_session.cc (right): https://codereview.chromium.org/2319343004/diff/60001/net/quic/chromium/quic_... net/quic/chromium/quic_chromium_client_session.cc:996: } On 2016/09/10 23:08:31, Jana wrote: > On 2016/09/10 16:29:46, Ryan Hamilton wrote: > > Under what circumstances is this code path hit? Can it happen from write > error? > > If so, as far as the connection is concerned, there is a write outstanding and > > the writer is write blocked. (Or at least it was when the original write error > > occurred). At this point, the connection is hanging around waiting for > > OnCanWrite to resume the write path. By calling SendPing here, we're actually > > causing a write in a place where the connection isn't expecting it. It might > all > > work correctly, but it feels strange. Perhaps this should do > > connection->OnCanWrite() before sending the ping? > > You're partially right that you can hit this code via the write error path, but > there's an important subtlety. > > This method can be called via a write error or via NCN events. If there's only > an NCN event that leads here, packet must be null and the connection is not > write blocked. If there's only a write error that leads here, then packet must > be non-null, since HandleWriteError stores a packet in the packet_ member. The > packet_ member is cleared in two cases: (i) after we post a task to call this > method with the packet, or (ii) num_migrations exceeded (see MigrateToSocket > below.) > > If the WriteError event and an NCN event are interleaved, leading to two > WriteToNewSockets being posted and executed, it doesn't matter which > WriteToNewSocket occurs first. The first one will rewrite packet_ and unblock > the connection (or close on error), and the second one will send a PING packet. > (The PING is not necessary, and I think I can tighten this -- I'll take a shot. > But this only happens under a very specific race -- NCN event posts a write task > after MigrateSessionOnWriteError completes but before WriteToNewSocket executes > -- and the cost is that an unnecessary PING packet is sent.) > > Make sense? > > Ok, coming back to this after some mode thinking -- the code didn't quite do > what I said it did, but I've fixed it now to do precisely what I describe above. > The problem was that MigrateToSocket was posting a WriteToNewSocket with a > packet parameter. Now, if WriteToNewSocket was posted by an NCN event first > (with packet=nullptr), and then a write error occurred, causing the writer to be > blocked before the NCN-event-posted WriteToNewSocket is executed, we could end > up in the situation you describe. This is hard to arrange in the test because I > need to carefully crank the message loop to arrange for these events to happen > in the order I described. (I'm trying to write tests with all possible > interleavings... but I think I'll need to use the test runner and do this by > hand.) > > I've changed the code to not post any packet parameter with WriteToNewSocket, > but for the method to decide on execution which packet to write. Meaning that > the packet_ member is populated at the beginning of the WriteError process, and > set to null at the end of it, in WriteToNewSocket. If an NCN-triggered > WriteToNewSocket shows up during this process, it will do the right thing of > writing packet_ instead of sending a ping. Ok, I've simplified and removed the possibility of multiple tasks executing WriteToNewSocket. The code now should now allow a write to proceed if a write posted earlier executes.
https://codereview.chromium.org/2319343004/diff/80001/net/quic/chromium/quic_... File net/quic/chromium/quic_chromium_client_session.cc (right): https://codereview.chromium.org/2319343004/diff/80001/net/quic/chromium/quic_... net/quic/chromium/quic_chromium_client_session.cc:1004: // not write blocked. If there's only a write error that leads I'm not sure this statement is correct. Consider the following sequence of events: T0: Write starts, returns ERR_IO_PENDING, writer is write blocked T1: NCN event fires which causes migration. Doesn't that results in a connection and session which are expecting OnCanWrite())? In thinking about this more... When migration happens, an new socket and a new writer are created. That new writer is not write blocked, obviously, because no writes have happened. I think that the simplest approach to this situation would be for MigrateToSocket() to call set_write_blocked(true) on the new writer. Then it stays blocked (without a write ever happening) until the posted task runs. Then when we get here, call set_write_blocked(false) and perform the IO via either: OnCanWrite() SendPing() or WritePacketToSocket() ... OnCanWrite() What do you think? https://codereview.chromium.org/2319343004/diff/80001/net/quic/chromium/quic_... File net/quic/chromium/quic_chromium_client_session.h (right): https://codereview.chromium.org/2319343004/diff/80001/net/quic/chromium/quic_... net/quic/chromium/quic_chromium_client_session.h:379: bool should_write_block_; // True if the writer should write block. Instead of doings this, it might just be easier to call set_write_blocked on the writer?
Thanks, Ryan, responses below. You may also want to look at the latest version of the code (patch set #6). https://codereview.chromium.org/2319343004/diff/80001/net/quic/chromium/quic_... File net/quic/chromium/quic_chromium_client_session.cc (right): https://codereview.chromium.org/2319343004/diff/80001/net/quic/chromium/quic_... net/quic/chromium/quic_chromium_client_session.cc:1004: // not write blocked. If there's only a write error that leads On 2016/09/11 15:38:42, Ryan Hamilton wrote: > I'm not sure this statement is correct. Consider the following sequence of > events: > > T0: Write starts, returns ERR_IO_PENDING, writer is write blocked > T1: NCN event fires which causes migration. > > Doesn't that results in a connection and session which are expecting > OnCanWrite())? > > In thinking about this more... When migration happens, an new socket and a new > writer are created. That new writer is not write blocked, obviously, because no > writes have happened. I think that the simplest approach to this situation would > be for MigrateToSocket() to call set_write_blocked(true) on the new writer. Then > it stays blocked (without a write ever happening) until the posted task runs. > Then when we get here, call set_write_blocked(false) and perform the IO via > either: > > OnCanWrite() > SendPing() > > or > > WritePacketToSocket() > ... > OnCanWrite() > > What do you think? So your example sequence of events is what I called interleaved events below... FWIW, the code does exactly what you are suggesting I think. should_write_block is set to false below and is set to true, though if you look at the latest version of the code, this variable name has changed to write_pending_. I also have a test to test for your example sequence of events (MigrationOnNetworkDisconnectedWithWriteErrorQueued), and things work as expected. I've also removed this detailed comment in teh new version of the code, but if you think it (or something like it) is useful, I'll bring it back. https://codereview.chromium.org/2319343004/diff/80001/net/quic/chromium/quic_... File net/quic/chromium/quic_chromium_client_session.h (right): https://codereview.chromium.org/2319343004/diff/80001/net/quic/chromium/quic_... net/quic/chromium/quic_chromium_client_session.h:379: bool should_write_block_; // True if the writer should write block. On 2016/09/11 15:38:42, Ryan Hamilton wrote: > Instead of doings this, it might just be easier to call set_write_blocked on the > writer? Given that the delegate is part of the process anyways for the writer, the current interaction doesn't seem unnatural... I'm inclined to leave it as is because the writer is using the delegate to block and all delegate logic is in the session. Though if you feel strongly, I'm happy to change it.
https://codereview.chromium.org/2319343004/diff/100001/net/quic/chromium/quic... File net/quic/chromium/quic_chromium_client_session.cc (right): https://codereview.chromium.org/2319343004/diff/100001/net/quic/chromium/quic... net/quic/chromium/quic_chromium_client_session.cc:1005: connection()->SendPing(); Maybe I'm not looking in the right place, but I don't see OnCanWrite here?
https://codereview.chromium.org/2319343004/diff/100001/net/quic/chromium/quic... File net/quic/chromium/quic_chromium_client_session.cc (right): https://codereview.chromium.org/2319343004/diff/100001/net/quic/chromium/quic... net/quic/chromium/quic_chromium_client_session.cc:1005: connection()->SendPing(); On 2016/09/11 19:24:12, Ryan Hamilton wrote: > Maybe I'm not looking in the right place, but I don't see OnCanWrite here? If there was a write block just before this WriteToNewSocket is executed, HandleWriteError is called, which sets packet_. So when this task gets scheduled, it will find packet_ to be non null, since it's the same member variable that HandleWriteError sets. (This task also sets write_pending_ and migration_pending_ to false above so that the migration task posted by HandleWriteError aborts itself when it runs.) What am I missing?
On 2016/09/11 19:44:05, Jana wrote: > https://codereview.chromium.org/2319343004/diff/100001/net/quic/chromium/quic... > File net/quic/chromium/quic_chromium_client_session.cc (right): > > https://codereview.chromium.org/2319343004/diff/100001/net/quic/chromium/quic... > net/quic/chromium/quic_chromium_client_session.cc:1005: > connection()->SendPing(); > On 2016/09/11 19:24:12, Ryan Hamilton wrote: > > Maybe I'm not looking in the right place, but I don't see OnCanWrite here? > > If there was a write block just before this WriteToNewSocket is executed, > HandleWriteError is called, which sets packet_. So when this task gets > scheduled, it will find packet_ to be non null, since it's the same member > variable that HandleWriteError sets. (This task also sets write_pending_ and > migration_pending_ to false above so that the migration task posted by > HandleWriteError aborts itself when it runs.) > > What am I missing? I'll go through your example scenario and a couple of alternative scenarios, and you can tell me where we aren't seeing eye-to-eye: You said: "T0: Write starts, returns ERR_IO_PENDING, writer is write blocked T1: NCN event fires which causes migration. Doesn't that results in a connection and session which are expecting OnCanWrite())?" Here are the events that will happen: T0: Write starts, returns ERR_IO_PENDING, writer is write blocked. Also, the packet is stored in packet_, migration_pending_ is set to true, and MigrateSessionOnWriteError is posted. T1: NCN event fires, which causes migration. This results in MigrateToSocket getting called, which sets migration_pending_ to false. Also WriteToNewSocket is posted and write_pending_ is set to true. T2: MigrateSessionOnWriteError (posted at T0) runs. Since migration_pending_ is false, this step returns. T3: WriteToNewSocket (posted at T2) runs. Since write_pending_ is still true, this step continues. packet_ is not null (populated at T0), is written to network, and OnCanWrite is called. Alteernative sequence 1: T0: Write starts, returns ERR_IO_PENDING, writer is write blocked. Also, the packet is stored in packet_, migration_pending_ is set to true, and MigrateSessionOnWriteError is posted. T1: MigrateSessionOnWriteError (posted at T0) is executed. Since migration_pending_ is true, this step continues. Results in MigrateToSocket getting called, which sets migration_pending_ to false. Also WriteToNewSocket is posted and write_pending_ is set to true. T2: NCN event fires, which causes migration. This results in MigrateToSocket getting called, which sets migration_pending_ to false. Also WriteToNewSocket is posted and write_pending_ is set to true. T3: WriteToNewSocket (posted at T1) runs. Since write_pending_ is still true, this step continues. packet_ is not null (populated at T0), is written to network, and OnCanWrite is called. Also sets both migration_pending_ and write_pending_ to false. T4: WriteToNewSocket (posted at T1) runs. Since write_pending_ is false, simply returns. Alteernative sequence 2: T0: NCN event fires, which causes migration. This results in MigrateToSocket getting called, which sets migration_pending_ to false (this was already false). WriteToNewSocket is posted and write_pending_ is set to true. T1: Write starts, returns ERR_IO_PENDING, writer is write blocked. Also, the packet is stored in packet_, migration_pending_ is set to true, and MigrateSessionOnWriteError is posted. T2: WriteToNewSocket (posted at T0) runs. Since write_pending_ is still true, this step continues. packet_ is not null (populated at T1), is written to network, and OnCanWrite is called. Also sets both migration_pending_ and write_pending_ to false. T3: MigrateSessionOnWriteError (posted at T1) is executed. Since migration_pending_ is false, this step returns. The only sequence where packet_ is null in WriteToNewSocket is when write error hasn't happened. Make sense?
On 2016/09/11 20:15:02, Jana wrote: > On 2016/09/11 19:44:05, Jana wrote: > > > https://codereview.chromium.org/2319343004/diff/100001/net/quic/chromium/quic... > > File net/quic/chromium/quic_chromium_client_session.cc (right): > > > > > https://codereview.chromium.org/2319343004/diff/100001/net/quic/chromium/quic... > > net/quic/chromium/quic_chromium_client_session.cc:1005: > > connection()->SendPing(); > > On 2016/09/11 19:24:12, Ryan Hamilton wrote: > > > Maybe I'm not looking in the right place, but I don't see OnCanWrite here? > > > > If there was a write block just before this WriteToNewSocket is executed, > > HandleWriteError is called, which sets packet_. So when this task gets > > scheduled, it will find packet_ to be non null, since it's the same member > > variable that HandleWriteError sets. (This task also sets write_pending_ and > > migration_pending_ to false above so that the migration task posted by > > HandleWriteError aborts itself when it runs.) > > > > What am I missing? > > I'll go through your example scenario and a couple of alternative scenarios, and > you can tell me where we aren't seeing eye-to-eye: > You said: > "T0: Write starts, returns ERR_IO_PENDING, writer is write blocked > T1: NCN event fires which causes migration. > Doesn't that results in a connection and session which are expecting > OnCanWrite())?" > > Here are the events that will happen: > T0: Write starts, returns ERR_IO_PENDING, writer is write blocked. Also, the > packet is stored in packet_, migration_pending_ is set to true, and > MigrateSessionOnWriteError is posted. > T1: NCN event fires, which causes migration. This results in MigrateToSocket > getting called, which sets migration_pending_ to false. Also WriteToNewSocket is > posted and write_pending_ is set to true. > T2: MigrateSessionOnWriteError (posted at T0) runs. Since migration_pending_ is > false, this step returns. > T3: WriteToNewSocket (posted at T2) runs. Since write_pending_ is still true, > this step continues. packet_ is not null (populated at T0), is written to > network, and OnCanWrite is called. > > Alteernative sequence 1: > T0: Write starts, returns ERR_IO_PENDING, writer is write blocked. Also, the > packet is stored in packet_, migration_pending_ is set to true, and > MigrateSessionOnWriteError is posted. > T1: MigrateSessionOnWriteError (posted at T0) is executed. Since > migration_pending_ is true, this step continues. Results in MigrateToSocket > getting called, which sets migration_pending_ to false. Also WriteToNewSocket is > posted and write_pending_ is set to true. > T2: NCN event fires, which causes migration. This results in MigrateToSocket > getting called, which sets migration_pending_ to false. Also WriteToNewSocket is > posted and write_pending_ is set to true. > T3: WriteToNewSocket (posted at T1) runs. Since write_pending_ is still true, > this step continues. packet_ is not null (populated at T0), is written to > network, and OnCanWrite is called. Also sets both migration_pending_ and > write_pending_ to false. > T4: WriteToNewSocket (posted at T1) runs. Since write_pending_ is false, simply > returns. > > Alteernative sequence 2: > T0: NCN event fires, which causes migration. This results in MigrateToSocket > getting called, which sets migration_pending_ to false (this was already false). > WriteToNewSocket is posted and write_pending_ is set to true. > T1: Write starts, returns ERR_IO_PENDING, writer is write blocked. Also, the > packet is stored in packet_, migration_pending_ is set to true, and > MigrateSessionOnWriteError is posted. > T2: WriteToNewSocket (posted at T0) runs. Since write_pending_ is still true, > this step continues. packet_ is not null (populated at T1), is written to > network, and OnCanWrite is called. Also sets both migration_pending_ and > write_pending_ to false. > T3: MigrateSessionOnWriteError (posted at T1) is executed. Since > migration_pending_ is false, this step returns. > > The only sequence where packet_ is null in WriteToNewSocket is when write error > hasn't happened. Make sense? I think the disconnect is that in the scenario I'm thinking of, there is no write error at all. merely an in-process async write when the NCN migration starts.
I think this is looking quite good. I need to head out and will be back this evening. https://codereview.chromium.org/2319343004/diff/80001/net/quic/chromium/quic_... File net/quic/chromium/quic_chromium_client_session.h (right): https://codereview.chromium.org/2319343004/diff/80001/net/quic/chromium/quic_... net/quic/chromium/quic_chromium_client_session.h:379: bool should_write_block_; // True if the writer should write block. On 2016/09/11 18:32:30, Jana wrote: > On 2016/09/11 15:38:42, Ryan Hamilton wrote: > > Instead of doings this, it might just be easier to call set_write_blocked on > the > > writer? > > Given that the delegate is part of the process anyways for the writer, the > current interaction doesn't seem unnatural... I'm inclined to leave it as is > because the writer is using the delegate to block and all delegate logic is in > the session. Though if you feel strongly, I'm happy to change it. That's a fair point. I think set_write_blocked() might be better (after noodling about it for a while) for a few reasons. 1) if we already have set_write_blocked we dont' really need a delegate method. 2) it requires even more state in the session (which is full of state already :>). 3) ShouldWriteBlock() might (via a bug) be intended to apply to writer A but we swap it out and it ends up applying with writer B. This is minor in the grand scheme of things, but with all the exciting complications of migration, I think it'd be slightly cleaner.
THanks, Ryan. I added a call to OnCanWrite(), and changed the delegate shouldwriteblock to a call into the writer. PTAL https://codereview.chromium.org/2319343004/diff/80001/net/quic/chromium/quic_... File net/quic/chromium/quic_chromium_client_session.h (right): https://codereview.chromium.org/2319343004/diff/80001/net/quic/chromium/quic_... net/quic/chromium/quic_chromium_client_session.h:379: bool should_write_block_; // True if the writer should write block. On 2016/09/11 20:44:24, Ryan Hamilton wrote: > On 2016/09/11 18:32:30, Jana wrote: > > On 2016/09/11 15:38:42, Ryan Hamilton wrote: > > > Instead of doings this, it might just be easier to call set_write_blocked on > > the > > > writer? > > > > Given that the delegate is part of the process anyways for the writer, the > > current interaction doesn't seem unnatural... I'm inclined to leave it as is > > because the writer is using the delegate to block and all delegate logic is in > > the session. Though if you feel strongly, I'm happy to change it. > > That's a fair point. I think set_write_blocked() might be better (after noodling > about it for a while) for a few reasons. 1) if we already have set_write_blocked > we dont' really need a delegate method. 2) it requires even more state in the > session (which is full of state already :>). 3) ShouldWriteBlock() might (via a > bug) be intended to apply to writer A but we swap it out and it ends up applying > with writer B. > > This is minor in the grand scheme of things, but with all the exciting > complications of migration, I think it'd be slightly cleaner. Fair enough. Done.
Added tests for async write before DISCONNECTED and MADE_DEFAULT notifications, and fixed code to only call OnCanWrite if writer is unblocked after SendPing().
OK, this looks great. I've not yet looked at the tests, but I think the logic is spot on. As you said earlier, this is much easier to reason about. I have a few nits, and one suggestion about getting rid of migration_pending_. I'll look at the tests this evening, but this is looking sweet. https://codereview.chromium.org/2319343004/diff/140001/net/quic/chromium/quic... File net/quic/chromium/quic_chromium_client_session.cc (right): https://codereview.chromium.org/2319343004/diff/140001/net/quic/chromium/quic... net/quic/chromium/quic_chromium_client_session.cc:969: migration_pending_ = true; This is the only place migration_pending_ is set to true, right? Looks like the only purpose of this is to prevent MigrateSessionOnWriteError from doing work if the NCN event fired. Do I have this right? If so perhaps a better name might be: need_to_migrate_for_write_error_ Or some such. (this is a nit ;>) Oh! Here's another possibly better idea. When you post the task for MigrateSessionOnError, pass in sockets_.size(). Then in MigrateSessionOnError check to see if the current sockets_.size() == the passed in value. Then you don't need an additional state variable. https://codereview.chromium.org/2319343004/diff/140001/net/quic/chromium/quic... net/quic/chromium/quic_chromium_client_session.cc:971: ->set_write_blocked(true); This is not needed is it? By returning ERR_IO_PENDING below the writer is guaranteed to be write blocked. https://codereview.chromium.org/2319343004/diff/140001/net/quic/chromium/quic... net/quic/chromium/quic_chromium_client_session.cc:987: return; nit: {}s needed. https://codereview.chromium.org/2319343004/diff/140001/net/quic/chromium/quic... net/quic/chromium/quic_chromium_client_session.cc:1012: // not leave the writer blocked. I think the best thing to do is to call OnCanWrite() before calling SendPing() unless there is some really compelling reason to do otherwise. Recall that the Async Packet Writer contract is to call OnCanWrite after a write completes async. So by calling SendPing first, we're sending a ping "in the middle" of sending the previous packet. https://codereview.chromium.org/2319343004/diff/140001/net/quic/chromium/quic... net/quic/chromium/quic_chromium_client_session.cc:1020: // reused via a write error. Avoid first person in comments: // Set packet to null before calling WritePacketToSocket because that method may set packet_ if there is a write error. (or somesuch) https://codereview.chromium.org/2319343004/diff/140001/net/quic/chromium/quic... net/quic/chromium/quic_chromium_client_session.cc:1036: DCHECK(result.error_code >= 0); nit: DCHECK_LT(0, result.error_code); https://codereview.chromium.org/2319343004/diff/140001/net/quic/chromium/quic... net/quic/chromium/quic_chromium_client_session.cc:1271: // socket. Also block the writer to prevent is being used until Can you add some language here which explains that the motivation is to avoid reentrancy issues if there is a write error? https://codereview.chromium.org/2319343004/diff/140001/net/quic/chromium/quic... net/quic/chromium/quic_chromium_client_session.cc:1272: // WriteToNewSocket completes. nit: move this sentence down to the call to block the writer so it's next to the code. https://codereview.chromium.org/2319343004/diff/140001/net/quic/chromium/quic... File net/quic/chromium/quic_chromium_client_session.h (right): https://codereview.chromium.org/2319343004/diff/140001/net/quic/chromium/quic... net/quic/chromium/quic_chromium_client_session.h:261: // else a PING packet is written. nit: s/else/otherwise/
Tests looking great! https://codereview.chromium.org/2319343004/diff/140001/net/quic/chromium/quic... File net/quic/chromium/quic_chromium_client_session.cc (right): https://codereview.chromium.org/2319343004/diff/140001/net/quic/chromium/quic... net/quic/chromium/quic_chromium_client_session.cc:999: return; You can take this or leave it... If we removed this early return and this method ended up firing twice (possible but unlikely) would the worst thing that would happen be that an extra ping gets sent? If so you might consider just dropping this variable altogether and skipping this check. Or totally keep it :> It's completely your call. https://codereview.chromium.org/2319343004/diff/140001/net/quic/chromium/quic... File net/quic/chromium/quic_chromium_client_session_test.cc (left): https://codereview.chromium.org/2319343004/diff/140001/net/quic/chromium/quic... net/quic/chromium/quic_chromium_client_session_test.cc:563: TEST_P(QuicChromiumClientSessionTest, MigrateToSocketWriteError) { Do we not need this test 'cause the real tests are in the stream factory test? https://codereview.chromium.org/2319343004/diff/140001/net/quic/chromium/quic... File net/quic/chromium/quic_stream_factory_test.cc (right): https://codereview.chromium.org/2319343004/diff/140001/net/quic/chromium/quic... net/quic/chromium/quic_stream_factory_test.cc:510: // Helper methods for tests of connection migration on write error. These methods all test some specific condition, right? In that case I'd probably start them with a prefix like TestMigrationOn.. TestMigrationOnWriteErrorNonMigratableStream TestMigrationOnWriteErrorMigrationDisabled TestMigrationOnMultipleWriteErrors TestMigrationOnNetworkDisconnected TestMigrationOnNetworkMadeDefault The other way to do this, by the way, is to extend QuicStreamFactoryTestBase for each of these and make it a TEST_P with appropriate Params. You approach is fine for now. Just something to think about down the road. https://codereview.chromium.org/2319343004/diff/140001/net/quic/chromium/quic... net/quic/chromium/quic_stream_factory_test.cc:511: void MigrateSessionOnWriteErrorNonMigratableStream(IoMode mode); I assume |mode| here is the write error io mode? perhaps write_error_mode? https://codereview.chromium.org/2319343004/diff/140001/net/quic/chromium/quic... net/quic/chromium/quic_stream_factory_test.cc:515: void MigrateSessionOnMultipleWriteErrors(IoMode mode1, IoMode mode2); Perhaps first_write_error_mode and second_write_error_mode? https://codereview.chromium.org/2319343004/diff/140001/net/quic/chromium/quic... net/quic/chromium/quic_stream_factory_test.cc:1480: } nit: Can you move the new tests below the helper method. Not that it really matters. (here and elsewhere) https://codereview.chromium.org/2319343004/diff/140001/net/quic/chromium/quic... net/quic/chromium/quic_stream_factory_test.cc:1495: socket_data.AddWrite(ASYNC, OK); If this is going to be a write of a ping packet, can you make this actually be an AddWrite with a PingPacket? (or does that cause problems 'cause the write never actually completes? https://codereview.chromium.org/2319343004/diff/140001/net/quic/chromium/quic... net/quic/chromium/quic_stream_factory_test.cc:2762: // between the two networks. nit wrapping is off https://codereview.chromium.org/2319343004/diff/140001/net/quic/chromium/quic... net/quic/chromium/quic_stream_factory_test.cc:2812: // stream->ReadResponseHeaders(callback_.callback())); Should this be commented back in? (it'd be great to have ERR_NETWORK_SOMETHING instead of PROTOCOL_ERROR if it's trivial)
Description was changed from ========== This CL refactors the code for migration on write error, making it asynchronous to avoid re-entrance issues. Also simplifies some methods in QuicStreamFactory. BUG= ========== to ========== This CL refactors the code for migration on write error, making it asynchronous to avoid reentrancyissues. Also simplifies some methods in QuicStreamFactory. BUG= ==========
Thanks Ryan -- comments addressed. PTAL. https://codereview.chromium.org/2319343004/diff/140001/net/quic/chromium/quic... File net/quic/chromium/quic_chromium_client_session.cc (right): https://codereview.chromium.org/2319343004/diff/140001/net/quic/chromium/quic... net/quic/chromium/quic_chromium_client_session.cc:969: migration_pending_ = true; On 2016/09/12 00:52:18, Ryan Hamilton wrote: > This is the only place migration_pending_ is set to true, right? Looks like the > only purpose of this is to prevent MigrateSessionOnWriteError from doing work if > the NCN event fired. Do I have this right? If so perhaps a better name might be: > > need_to_migrate_for_write_error_ > > Or some such. (this is a nit ;>) This is used (later in the pause CL) by the NCN code path as well, so this won't be a good name then. > Oh! Here's another possibly better idea. When you post the task for > MigrateSessionOnError, pass in sockets_.size(). Then in MigrateSessionOnError > check to see if the current sockets_.size() == the passed in value. Then you > don't need an additional state variable. Per conversation, leaving this for later. https://codereview.chromium.org/2319343004/diff/140001/net/quic/chromium/quic... net/quic/chromium/quic_chromium_client_session.cc:971: ->set_write_blocked(true); On 2016/09/12 00:52:18, Ryan Hamilton wrote: > This is not needed is it? By returning ERR_IO_PENDING below the writer is > guaranteed to be write blocked. Oh of course -- I am not sure why I added this here. Removed. https://codereview.chromium.org/2319343004/diff/140001/net/quic/chromium/quic... net/quic/chromium/quic_chromium_client_session.cc:987: return; On 2016/09/12 00:52:18, Ryan Hamilton wrote: > nit: {}s needed. Done. https://codereview.chromium.org/2319343004/diff/140001/net/quic/chromium/quic... net/quic/chromium/quic_chromium_client_session.cc:999: return; On 2016/09/12 01:32:57, Ryan Hamilton wrote: > You can take this or leave it... If we removed this early return and this method > ended up firing twice (possible but unlikely) would the worst thing that would > happen be that an extra ping gets sent? If so you might consider just dropping > this variable altogether and skipping this check. > > Or totally keep it :> It's completely your call. I did have the code doing that earlier, but enforcing one packet instead of two makes reasoning about behavior consistent (If there's a write error, packet is resent, otherwise PING is sent), and that helps with keeping tests more consistent across interleaved write error + NCN events. I'd prefer to keep it. https://codereview.chromium.org/2319343004/diff/140001/net/quic/chromium/quic... net/quic/chromium/quic_chromium_client_session.cc:1012: // not leave the writer blocked. On 2016/09/12 00:52:18, Ryan Hamilton wrote: > I think the best thing to do is to call OnCanWrite() before calling SendPing() > unless there is some really compelling reason to do otherwise. Recall that the > Async Packet Writer contract is to call OnCanWrite after a write completes > async. So by calling SendPing first, we're sending a ping "in the middle" of > sending the previous packet. Hm. Yeah, this does make reasoning simpler. Done. https://codereview.chromium.org/2319343004/diff/140001/net/quic/chromium/quic... net/quic/chromium/quic_chromium_client_session.cc:1020: // reused via a write error. On 2016/09/12 00:52:18, Ryan Hamilton wrote: > Avoid first person in comments: > > // Set packet to null before calling WritePacketToSocket because that method > may set packet_ if there is a write error. > > (or somesuch) Done. https://codereview.chromium.org/2319343004/diff/140001/net/quic/chromium/quic... net/quic/chromium/quic_chromium_client_session.cc:1036: DCHECK(result.error_code >= 0); On 2016/09/12 00:52:18, Ryan Hamilton wrote: > nit: DCHECK_LT(0, result.error_code); Done. https://codereview.chromium.org/2319343004/diff/140001/net/quic/chromium/quic... net/quic/chromium/quic_chromium_client_session.cc:1271: // socket. Also block the writer to prevent is being used until On 2016/09/12 00:52:18, Ryan Hamilton wrote: > Can you add some language here which explains that the motivation is to avoid > reentrancy issues if there is a write error? Done. https://codereview.chromium.org/2319343004/diff/140001/net/quic/chromium/quic... net/quic/chromium/quic_chromium_client_session.cc:1272: // WriteToNewSocket completes. On 2016/09/12 00:52:18, Ryan Hamilton wrote: > nit: move this sentence down to the call to block the writer so it's next to the > code. Done. https://codereview.chromium.org/2319343004/diff/140001/net/quic/chromium/quic... File net/quic/chromium/quic_chromium_client_session.h (right): https://codereview.chromium.org/2319343004/diff/140001/net/quic/chromium/quic... net/quic/chromium/quic_chromium_client_session.h:261: // else a PING packet is written. On 2016/09/12 00:52:18, Ryan Hamilton wrote: > nit: s/else/otherwise/ Done. https://codereview.chromium.org/2319343004/diff/140001/net/quic/chromium/quic... File net/quic/chromium/quic_chromium_client_session_test.cc (left): https://codereview.chromium.org/2319343004/diff/140001/net/quic/chromium/quic... net/quic/chromium/quic_chromium_client_session_test.cc:563: TEST_P(QuicChromiumClientSessionTest, MigrateToSocketWriteError) { On 2016/09/12 01:32:57, Ryan Hamilton wrote: > Do we not need this test 'cause the real tests are in the stream factory test? Right, exactly. https://codereview.chromium.org/2319343004/diff/140001/net/quic/chromium/quic... File net/quic/chromium/quic_stream_factory_test.cc (right): https://codereview.chromium.org/2319343004/diff/140001/net/quic/chromium/quic... net/quic/chromium/quic_stream_factory_test.cc:510: // Helper methods for tests of connection migration on write error. On 2016/09/12 01:32:57, Ryan Hamilton wrote: > These methods all test some specific condition, right? In that case I'd probably > start them with a prefix like > > TestMigrationOn.. > > TestMigrationOnWriteErrorNonMigratableStream > TestMigrationOnWriteErrorMigrationDisabled > TestMigrationOnMultipleWriteErrors > TestMigrationOnNetworkDisconnected > TestMigrationOnNetworkMadeDefault > > The other way to do this, by the way, is to extend QuicStreamFactoryTestBase for > each of these and make it a TEST_P with appropriate Params. You approach is fine > for now. Just something to think about down the road. Changed to TestMigrationOn. I thought about TEST_P, but this file already runs ~1200 tests, and these combinations do not make sense for most tests, so I used the poor man's method. https://codereview.chromium.org/2319343004/diff/140001/net/quic/chromium/quic... net/quic/chromium/quic_stream_factory_test.cc:511: void MigrateSessionOnWriteErrorNonMigratableStream(IoMode mode); On 2016/09/12 01:32:58, Ryan Hamilton wrote: > I assume |mode| here is the write error io mode? perhaps write_error_mode? Done. https://codereview.chromium.org/2319343004/diff/140001/net/quic/chromium/quic... net/quic/chromium/quic_stream_factory_test.cc:515: void MigrateSessionOnMultipleWriteErrors(IoMode mode1, IoMode mode2); On 2016/09/12 01:32:57, Ryan Hamilton wrote: > Perhaps first_write_error_mode and second_write_error_mode? Done. https://codereview.chromium.org/2319343004/diff/140001/net/quic/chromium/quic... net/quic/chromium/quic_stream_factory_test.cc:1480: } On 2016/09/12 01:32:57, Ryan Hamilton wrote: > nit: Can you move the new tests below the helper method. Not that it really > matters. (here and elsewhere) Done. https://codereview.chromium.org/2319343004/diff/140001/net/quic/chromium/quic... net/quic/chromium/quic_stream_factory_test.cc:1495: socket_data.AddWrite(ASYNC, OK); On 2016/09/12 01:32:58, Ryan Hamilton wrote: > If this is going to be a write of a ping packet, can you make this actually be > an AddWrite with a PingPacket? (or does that cause problems 'cause the write > never actually completes? Right exactly, I don't know how to make it expect a PING packet on an ASYNC write. https://codereview.chromium.org/2319343004/diff/140001/net/quic/chromium/quic... net/quic/chromium/quic_stream_factory_test.cc:2762: // between the two networks. On 2016/09/12 01:32:57, Ryan Hamilton wrote: > nit wrapping is off Ah -- sent comment > 80 columns through git cl format. Fixed. https://codereview.chromium.org/2319343004/diff/140001/net/quic/chromium/quic... net/quic/chromium/quic_stream_factory_test.cc:2812: // stream->ReadResponseHeaders(callback_.callback())); On 2016/09/12 01:32:57, Ryan Hamilton wrote: > Should this be commented back in? > > (it'd be great to have ERR_NETWORK_SOMETHING instead of PROTOCOL_ERROR if it's > trivial) Fixed test to be more sensible. I don't think this should be ERR_NETWORK_SOMETHING since it's basically a write error. At the moment, all write errors are collapsed into QUIC_PROTOCOL_ERROR.
lgtm https://codereview.chromium.org/2319343004/diff/240001/net/quic/chromium/quic... File net/quic/chromium/quic_chromium_client_session.cc (right): https://codereview.chromium.org/2319343004/diff/240001/net/quic/chromium/quic... net/quic/chromium/quic_chromium_client_session.cc:974: } I'm guessing we don't have tests for this codepath. Can you make sure these get added in a followup? https://codereview.chromium.org/2319343004/diff/240001/net/quic/chromium/quic... net/quic/chromium/quic_chromium_client_session.cc:1293: ->set_write_blocked(true); nit: can you move this above 1281, and just call writer->set() (avoid getting the writer back out of the connection and casting)
Thanks much, Ryan. Responses inline. https://codereview.chromium.org/2319343004/diff/240001/net/quic/chromium/quic... File net/quic/chromium/quic_chromium_client_session.cc (right): https://codereview.chromium.org/2319343004/diff/240001/net/quic/chromium/quic... net/quic/chromium/quic_chromium_client_session.cc:974: } On 2016/09/12 22:13:55, Ryan Hamilton wrote: > I'm guessing we don't have tests for this codepath. > > Can you make sure these get added in a followup? Will do. I've created a tracking bug for followup work, and I'll add this in. (https://bugs.chromium.org/p/chromium/issues/detail?id=646145) https://codereview.chromium.org/2319343004/diff/240001/net/quic/chromium/quic... net/quic/chromium/quic_chromium_client_session.cc:1293: ->set_write_blocked(true); On 2016/09/12 22:13:55, Ryan Hamilton wrote: > nit: can you move this above 1281, and just call writer->set() (avoid getting > the writer back out of the connection and casting) Done.
The CQ bit was checked by jri@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from rch@chromium.org Link to the patchset: https://codereview.chromium.org/2319343004/#ps280001 (title: "synced and rebased")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== This CL refactors the code for migration on write error, making it asynchronous to avoid reentrancyissues. Also simplifies some methods in QuicStreamFactory. BUG= ========== to ========== This CL refactors the code for migration on write error, making it asynchronous to avoid reentrancyissues. Also simplifies some methods in QuicStreamFactory. BUG=646145 ==========
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
The CQ bit was checked by jri@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== This CL refactors the code for migration on write error, making it asynchronous to avoid reentrancyissues. Also simplifies some methods in QuicStreamFactory. BUG=646145 ========== to ========== This CL refactors the code for migration on write error, making it asynchronous to avoid reentrancyissues. Also simplifies some methods in QuicStreamFactory. BUG=646145 ==========
Message was sent while issue was closed.
Committed patchset #15 (id:280001)
Message was sent while issue was closed.
Description was changed from ========== This CL refactors the code for migration on write error, making it asynchronous to avoid reentrancyissues. Also simplifies some methods in QuicStreamFactory. BUG=646145 ========== to ========== This CL refactors the code for migration on write error, making it asynchronous to avoid reentrancyissues. Also simplifies some methods in QuicStreamFactory. BUG=646145 Committed: https://crrev.com/9f3037153a2feb63bbaeaf07ab35c8ff9a3096ad Cr-Commit-Position: refs/heads/master@{#418128} ==========
Message was sent while issue was closed.
Patchset 15 (id:??) landed as https://crrev.com/9f3037153a2feb63bbaeaf07ab35c8ff9a3096ad Cr-Commit-Position: refs/heads/master@{#418128} |
