Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1113203002:
Convert DnsTransactionTest to use SequencedSocketData (Closed)
|
Created:
5 years, 7 months ago by Ryan Hamilton Modified:
5 years, 7 months ago Reviewers:
mmenke CC:
chromium-reviews, cbentzel+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionConvert DnsTransactionTest to use SequencedSocketData
BUG=
Committed: https://crrev.com/8fd741f2eeb8b6cc59a780b81b4a53db269f6454
Cr-Commit-Position: refs/heads/master@{#330293}
Patch Set 1 #Patch Set 2 : Rebase #
Total comments: 7
Patch Set 3 : fix comments #Patch Set 4 : final fixes #
Total comments: 7
Patch Set 5 : ~MockTCPClientSocket #Patch Set 6 : undo ~MockTCPClientSocket #Patch Set 7 : Rebase #Patch Set 8 : Rebase and tweak QUIC #Patch Set 9 : Rebase and tweak #
Messages
Total messages: 29 (7 generated)
rch@chromium.org changed reviewers: + mmenke@chromium.org
https://codereview.chromium.org/1113203002/diff/20001/net/dns/dns_transaction... File net/dns/dns_transaction_unittest.cc (right): https://codereview.chromium.org/1113203002/diff/20001/net/dns/dns_transaction... net/dns/dns_transaction_unittest.cc:132: return provider_->WasDataWritten(); optional: Seems a little weird for a lower-case named method to be calling a capitalized one. Suggest either renaming this, or renaming and inlining was_data_written. Latter seems simpler. Google C++ style guide now says "You may also use lowercase letters for other very short inlined functions. For example if a function were so cheap you would not cache the value if you were calling it in a loop, then lowercase naming would be acceptable." That doesn't completely disallow this, but I'm assuming this is using lowercase as an instance of the example, which does make it weird. https://codereview.chromium.org/1113203002/diff/20001/net/dns/dns_transaction... net/dns/dns_transaction_unittest.cc:132: return provider_->WasDataWritten(); For the TCP case, where there are two writes, this isn't write...err...right. WasAllDataWritten? Or just call base::RunLoop().RunAllPending() first (Which is ugly, but works). https://codereview.chromium.org/1113203002/diff/20001/net/dns/dns_transaction... net/dns/dns_transaction_unittest.cc:136: size_t num_reads_and_writes() { return reads_.size() + writes_.size(); } nit: const
Thanks! https://codereview.chromium.org/1113203002/diff/20001/net/dns/dns_transaction... File net/dns/dns_transaction_unittest.cc (right): https://codereview.chromium.org/1113203002/diff/20001/net/dns/dns_transaction... net/dns/dns_transaction_unittest.cc:132: return provider_->WasDataWritten(); On 2015/05/04 19:10:57, mmenke wrote: > optional: Seems a little weird for a lower-case named method to be calling a > capitalized one. Suggest either renaming this, or renaming and inlining > was_data_written. Latter seems simpler. > > Google C++ style guide now says "You may also use lowercase letters for other > very short inlined functions. For example if a function were so cheap you would > not cache the value if you were calling it in a loop, then lowercase naming > would be acceptable." > > That doesn't completely disallow this, but I'm assuming this is using lowercase > as an instance of the example, which does make it weird. Good point! I've inlined WasDataWritten(). https://codereview.chromium.org/1113203002/diff/20001/net/dns/dns_transaction... net/dns/dns_transaction_unittest.cc:136: size_t num_reads_and_writes() { return reads_.size() + writes_.size(); } On 2015/05/04 19:10:57, mmenke wrote: > nit: const Done.
On 2015/05/04 19:20:30, Ryan Hamilton wrote: > Thanks! > > https://codereview.chromium.org/1113203002/diff/20001/net/dns/dns_transaction... > File net/dns/dns_transaction_unittest.cc (right): > > https://codereview.chromium.org/1113203002/diff/20001/net/dns/dns_transaction... > net/dns/dns_transaction_unittest.cc:132: return provider_->WasDataWritten(); > On 2015/05/04 19:10:57, mmenke wrote: > > optional: Seems a little weird for a lower-case named method to be calling a > > capitalized one. Suggest either renaming this, or renaming and inlining > > was_data_written. Latter seems simpler. > > > > Google C++ style guide now says "You may also use lowercase letters for other > > very short inlined functions. For example if a function were so cheap you > would > > not cache the value if you were calling it in a loop, then lowercase naming > > would be acceptable." > > > > That doesn't completely disallow this, but I'm assuming this is using > lowercase > > as an instance of the example, which does make it weird. > > Good point! I've inlined WasDataWritten(). > > https://codereview.chromium.org/1113203002/diff/20001/net/dns/dns_transaction... > net/dns/dns_transaction_unittest.cc:136: size_t num_reads_and_writes() { return > reads_.size() + writes_.size(); } > On 2015/05/04 19:10:57, mmenke wrote: > > nit: const > > Done. I'm assuming you did see my third comment? (Ok, I'm doing no such thing)
Whoops! Sorry, the two comments were on the same line of code, and I totally spaced on the first. https://codereview.chromium.org/1113203002/diff/20001/net/dns/dns_transaction... File net/dns/dns_transaction_unittest.cc (right): https://codereview.chromium.org/1113203002/diff/20001/net/dns/dns_transaction... net/dns/dns_transaction_unittest.cc:132: return provider_->WasDataWritten(); On 2015/05/04 19:10:57, mmenke wrote: > For the TCP case, where there are two writes, this isn't write...err...right. > WasAllDataWritten? Or just call base::RunLoop().RunAllPending() first (Which is > ugly, but works). Whoops, sorry, missed this. I don't think I changed the semantics here, do you? In the old code, we simply checked that write index was > 0, which happens as soon as the write happen (even if it returns async). Now, the write index doesn't move until the callback completes so we need to change the implementation. Do you think I changed the semantics, or do you think that this was a latent bug? (What would calling RunAllPending do?)
https://codereview.chromium.org/1113203002/diff/20001/net/dns/dns_transaction... File net/dns/dns_transaction_unittest.cc (right): https://codereview.chromium.org/1113203002/diff/20001/net/dns/dns_transaction... net/dns/dns_transaction_unittest.cc:132: return provider_->WasDataWritten(); On 2015/05/04 19:24:46, Ryan Hamilton wrote: > On 2015/05/04 19:10:57, mmenke wrote: > > For the TCP case, where there are two writes, this isn't write...err...right. > > WasAllDataWritten? Or just call base::RunLoop().RunAllPending() first (Which > is > > ugly, but works). > > Whoops, sorry, missed this. I don't think I changed the semantics here, do you? > In the old code, we simply checked that write index was > 0, which happens as > soon as the write happen (even if it returns async). Now, the write index > doesn't move until the callback completes so we need to change the > implementation. Do you think I changed the semantics, or do you think that this > was a latent bug? Ahh...You're right! I saw the "at_write_eof" just below WasDataWritten(), and was assuming that's what the old code did. I think this was a bug in the test. This CL is small enough that I'd like to see if we can fix here, if we can do so easily and you don't mind (Since these tests are probably never going to be updated otherwise). If you prefer not to, I can live with that. > (What would calling RunAllPending do?) If we aren't at the "write eof", because there's a pending write, it would cause the write to complete, so we *would* be at the write eof, so you can then just use at_write_eof (And can probably check the read one, too, though that's less important).
Patchset #4 (id:60001) has been deleted
On 2015/05/04 19:34:50, mmenke wrote: > https://codereview.chromium.org/1113203002/diff/20001/net/dns/dns_transaction... > File net/dns/dns_transaction_unittest.cc (right): > > https://codereview.chromium.org/1113203002/diff/20001/net/dns/dns_transaction... > net/dns/dns_transaction_unittest.cc:132: return provider_->WasDataWritten(); > On 2015/05/04 19:24:46, Ryan Hamilton wrote: > > On 2015/05/04 19:10:57, mmenke wrote: > > > For the TCP case, where there are two writes, this isn't > write...err...right. > > > WasAllDataWritten? Or just call base::RunLoop().RunAllPending() first > (Which > > is > > > ugly, but works). > > > > Whoops, sorry, missed this. I don't think I changed the semantics here, do > you? > > In the old code, we simply checked that write index was > 0, which happens as > > soon as the write happen (even if it returns async). Now, the write index > > doesn't move until the callback completes so we need to change the > > implementation. Do you think I changed the semantics, or do you think that > this > > was a latent bug? > > Ahh...You're right! I saw the "at_write_eof" just below WasDataWritten(), and > was assuming that's what the old code did. I think this was a bug in the test. > This CL is small enough that I'd like to see if we can fix here, if we can do so > easily and you don't mind (Since these tests are probably never going to be > updated otherwise). If you prefer not to, I can live with that. > > > (What would calling RunAllPending do?) > > If we aren't at the "write eof", because there's a pending write, it would cause > the write to complete, so we *would* be at the write eof, so you can then just > use at_write_eof (And can probably check the read one, too, though that's less > important). Hm. Ok, what you suggested seems simple enough. It did require a small change in the behavior of SequencedSocketData::OnWriteComplete which is to advance to the next state even if there is no socket. But that seems reasonable. Does this look good to you? (The gotcha here is that the CancelLookup test ends up resetting the transaction (and deleting the socket) while the write callback is pending. if we run the message loop, then the transaction completes, which is a test failure.)
https://codereview.chromium.org/1113203002/diff/80001/net/dns/dns_transaction... File net/dns/dns_transaction_unittest.cc (right): https://codereview.chromium.org/1113203002/diff/80001/net/dns/dns_transaction... net/dns/dns_transaction_unittest.cc:454: EXPECT_TRUE(socket_data_[i]->GetProvider()->AllWriteDataConsumed()) << i; No such method... https://codereview.chromium.org/1113203002/diff/80001/net/socket/socket_test_... File net/socket/socket_test_util.cc (right): https://codereview.chromium.org/1113203002/diff/80001/net/socket/socket_test_... net/socket/socket_test_util.cc:1724: data_->set_socket(nullptr); This seems reasonable. Should probably do it in ~MockTCPClientSocket as well.
https://codereview.chromium.org/1113203002/diff/80001/net/dns/dns_transaction... File net/dns/dns_transaction_unittest.cc (right): https://codereview.chromium.org/1113203002/diff/80001/net/dns/dns_transaction... net/dns/dns_transaction_unittest.cc:454: EXPECT_TRUE(socket_data_[i]->GetProvider()->AllWriteDataConsumed()) << i; On 2015/05/04 20:08:32, mmenke wrote: > No such method... Pretty sure there is :>. I added it in this CL: https://codereview.chromium.org/1114383003/ (Which I landed after our email exchange about needing a common base class for Static and Sequenced socket data) I verified that this version of the patch built after I rebased onto tip of trunk https://codereview.chromium.org/1113203002/diff/80001/net/socket/socket_test_... File net/socket/socket_test_util.cc (right): https://codereview.chromium.org/1113203002/diff/80001/net/socket/socket_test_... net/socket/socket_test_util.cc:1724: data_->set_socket(nullptr); On 2015/05/04 20:08:32, mmenke wrote: > This seems reasonable. Should probably do it in ~MockTCPClientSocket as well. Done.
https://codereview.chromium.org/1113203002/diff/80001/net/dns/dns_transaction... File net/dns/dns_transaction_unittest.cc (right): https://codereview.chromium.org/1113203002/diff/80001/net/dns/dns_transaction... net/dns/dns_transaction_unittest.cc:454: EXPECT_TRUE(socket_data_[i]->GetProvider()->AllWriteDataConsumed()) << i; On 2015/05/04 20:12:33, Ryan Hamilton wrote: > On 2015/05/04 20:08:32, mmenke wrote: > > No such method... > > Pretty sure there is :>. I added it in this CL: > > https://codereview.chromium.org/1114383003/ > > (Which I landed after our email exchange about needing a common base class for > Static and Sequenced socket data) > > I verified that this version of the patch built after I rebased onto tip of > trunk Erm...this CL doesn't touch any header files....
https://codereview.chromium.org/1113203002/diff/80001/net/dns/dns_transaction... File net/dns/dns_transaction_unittest.cc (right): https://codereview.chromium.org/1113203002/diff/80001/net/dns/dns_transaction... net/dns/dns_transaction_unittest.cc:454: EXPECT_TRUE(socket_data_[i]->GetProvider()->AllWriteDataConsumed()) << i; On 2015/05/04 20:14:11, mmenke wrote: > On 2015/05/04 20:12:33, Ryan Hamilton wrote: > > On 2015/05/04 20:08:32, mmenke wrote: > > > No such method... > > > > Pretty sure there is :>. I added it in this CL: > > > > https://codereview.chromium.org/1114383003/ > > > > (Which I landed after our email exchange about needing a common base class for > > Static and Sequenced socket data) > > > > I verified that this version of the patch built after I rebased onto tip of > > trunk > > Erm...this CL doesn't touch any header files.... Oops, missed the link. So used to ignoring links at the end of codereview emails that I missed it.
https://codereview.chromium.org/1113203002/diff/80001/net/socket/socket_test_... File net/socket/socket_test_util.cc (right): https://codereview.chromium.org/1113203002/diff/80001/net/socket/socket_test_... net/socket/socket_test_util.cc:1724: data_->set_socket(nullptr); On 2015/05/04 20:12:33, Ryan Hamilton wrote: > On 2015/05/04 20:08:32, mmenke wrote: > > This seems reasonable. Should probably do it in ~MockTCPClientSocket as well. > > Done. Actually, I take it back. Doing that causes other tests to crash. I think we have some lifetime issues between sockets and socket data which is under specified. Perhaps something to fix in a followup.
LGTM!
On 2015/05/04 20:15:09, Ryan Hamilton wrote: > https://codereview.chromium.org/1113203002/diff/80001/net/socket/socket_test_... > File net/socket/socket_test_util.cc (right): > > https://codereview.chromium.org/1113203002/diff/80001/net/socket/socket_test_... > net/socket/socket_test_util.cc:1724: data_->set_socket(nullptr); > On 2015/05/04 20:12:33, Ryan Hamilton wrote: > > On 2015/05/04 20:08:32, mmenke wrote: > > > This seems reasonable. Should probably do it in ~MockTCPClientSocket as > well. > > > > Done. > > Actually, I take it back. Doing that causes other tests to crash. > > I think we have some lifetime issues between sockets and socket data which is > under specified. Perhaps something to fix in a followup. That's fine - still LGTM as-is.
On 2015/05/04 20:15:02, mmenke wrote: > > Oops, missed the link. So used to ignoring links at the end of codereview > emails that I missed it. Hah! That's awesome! It's amazing what a bit of conditioning does to our brains....
On 2015/05/04 20:15:59, mmenke wrote: > On 2015/05/04 20:15:09, Ryan Hamilton wrote: > > > https://codereview.chromium.org/1113203002/diff/80001/net/socket/socket_test_... > > File net/socket/socket_test_util.cc (right): > > > > > https://codereview.chromium.org/1113203002/diff/80001/net/socket/socket_test_... > > net/socket/socket_test_util.cc:1724: data_->set_socket(nullptr); > > On 2015/05/04 20:12:33, Ryan Hamilton wrote: > > > On 2015/05/04 20:08:32, mmenke wrote: > > > > This seems reasonable. Should probably do it in ~MockTCPClientSocket as > > well. > > > > > > Done. > > > > Actually, I take it back. Doing that causes other tests to crash. > > > > I think we have some lifetime issues between sockets and socket data which is > > under specified. Perhaps something to fix in a followup. > > That's fine - still LGTM as-is. Thanks! Will land w/o the ~MockTCPClientSocket code.
The CQ bit was checked by rch@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from mmenke@chromium.org Link to the patchset: https://codereview.chromium.org/1113203002/#ps120001 (title: "undo ~MockTCPClientSocket")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1113203002/120001
The CQ bit was unchecked by rch@chromium.org
On 2015/05/04 20:18:47, I haz the power (commit-bot) wrote: > CQ is trying da patch. Follow status at > https://chromium-cq-status.appspot.com/patch-status/1113203002/120001 Planning on landing this CL? Did the CQ just die on it?
On 2015/05/06 19:00:03, mmenke wrote: > On 2015/05/04 20:18:47, I haz the power (commit-bot) wrote: > > CQ is trying da patch. Follow status at > > https://chromium-cq-status.appspot.com/patch-status/1113203002/120001 > > Planning on landing this CL? Did the CQ just die on it? I'm in hell! I am planning to land the CL but the asan bot point out a use after free. Fixing this led to a different CL in which I made the sockets and the data providers notify each other on destruction. This solved the problem for all but one test which exploded. I need to track that down. But in the mean time, I'm unable to land this trivial CL: https://codereview.chromium.org/1114383003/ because some iOS bot explodes in a way I don't even remotely understand. If you're bored I'd love to brainstorm with you about that CL.
The CQ bit was checked by rch@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from mmenke@chromium.org Link to the patchset: https://codereview.chromium.org/1113203002/#ps180001 (title: "Rebase and tweak")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1113203002/180001
Message was sent while issue was closed.
Committed patchset #9 (id:180001)
Message was sent while issue was closed.
Patchset 9 (id:??) landed as https://crrev.com/8fd741f2eeb8b6cc59a780b81b4a53db269f6454 Cr-Commit-Position: refs/heads/master@{#330293} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
