Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2369373002:
LogDnsClient now returns some errors synchronously (Closed)
|
Created:
4 years, 2 months ago by Rob Percival Modified:
4 years, 2 months ago CC:
chromium-reviews, rsleevi+watch_chromium.org, certificate-transparency-chrome_googlegroups.com, Eran Messeri Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionLogDnsClient now returns some errors synchronously
This allows users to handle issues, such as throttling, immediately.
Uses a sync-async DoLoop pattern internally now.
BUG=624894
Committed: https://crrev.com/cfe43126e0716d7626175dd0c4cac9a12863a50b
Cr-Commit-Position: refs/heads/master@{#426451}
Patch Set 1 #
Total comments: 18
Patch Set 2 : Rebase #Patch Set 3 : Addresses Eran's comments #Patch Set 4 : Change int to net::Error #
Total comments: 16
Patch Set 5 : Rebase and minor refactoring #Patch Set 6 : Addresses Eran's comments #
Total comments: 27
Patch Set 7 : Rebase #Patch Set 8 : Addresses some of Ryan's comments #Patch Set 9 : Rebase #Patch Set 10 : Rebase #
Total comments: 6
Patch Set 11 : Rebase #Patch Set 12 : Emulate HostResolver more closely #
Total comments: 8
Patch Set 13 : Rebase #Patch Set 14 : Addresses Eran's comments #
Messages
Total messages: 67 (53 generated)
The CQ bit was checked by robpercival@chromium.org to run a CQ dry run
robpercival@chromium.org changed reviewers: + eranm@chromium.org, rsleevi@chromium.org
PTAL.
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
The CQ bit was checked by robpercival@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...)
Overall design and implementation of the DoLoop pattern seems solid, left a few minor comments. https://codereview.chromium.org/2369373002/diff/1/components/certificate_tran... File components/certificate_transparency/log_dns_client.cc (right): https://codereview.chromium.org/2369373002/diff/1/components/certificate_tran... components/certificate_transparency/log_dns_client.cc:109: // If a query was already in progress, this will abort it first. How often do you expect that case to happen? If not at all (i.e. starting a query twice is a programming error, then DCHECK that the next_state_ is State::NONE and then you don't have to worry about cancelling the current_dns_transaction_. https://codereview.chromium.org/2369373002/diff/1/components/certificate_tran... components/certificate_transparency/log_dns_client.cc:176: if (net_error != net::ERR_IO_PENDING) { Nit: Document why not doing anything in case of net_error being ERR_IO_PENDING is OK. https://codereview.chromium.org/2369373002/diff/1/components/certificate_tran... components/certificate_transparency/log_dns_client.cc:196: net::DnsTransactionFactory::CallbackType transaction_callback = Inline transaction_callback creation & use into CreateTransaction call. https://codereview.chromium.org/2369373002/diff/1/components/certificate_tran... components/certificate_transparency/log_dns_client.cc:211: // If we receive no response but no net::error either (shouldn't happen), "shouldn't happen" implies this is a programming error, right? If so, it seems a DCHECK for this case is more appropriate. https://codereview.chromium.org/2369373002/diff/1/components/certificate_tran... components/certificate_transparency/log_dns_client.cc:269: net::DnsTransactionFactory::CallbackType transaction_callback = Ditto, inline callback creation. https://codereview.chromium.org/2369373002/diff/1/components/certificate_tran... components/certificate_transparency/log_dns_client.cc:320: // The size of the CT log's tree. Add: from which the proof is requested. https://codereview.chromium.org/2369373002/diff/1/components/certificate_tran... File components/certificate_transparency/log_dns_client.h (right): https://codereview.chromium.org/2369373002/diff/1/components/certificate_tran... components/certificate_transparency/log_dns_client.h:70: // The |callback| is invoked when the query is complete, or an error occurs. Document the return value - that on successful submission ERR_IO_PENDING will be returned, another error code to indicate throttling (and everything else is a hard-fail). https://codereview.chromium.org/2369373002/diff/1/components/certificate_tran... components/certificate_transparency/log_dns_client.h:71: int QueryAuditProof(base::StringPiece domain_for_log, Question to rsleevi: Why not use net::Error here? https://codereview.chromium.org/2369373002/diff/1/components/certificate_tran... components/certificate_transparency/log_dns_client.h:103: // efficient. Be more explicit in the comment: // A FIFO queue of queries. Since entries will always be appended to the // end of the queue and lookups will yield entries at the beginning, // std::list is the efficient choice. https://codereview.chromium.org/2369373002/diff/1/components/certificate_tran... File components/certificate_transparency/log_dns_client_unittest.cc (right): https://codereview.chromium.org/2369373002/diff/1/components/certificate_tran... components/certificate_transparency/log_dns_client_unittest.cc:182: } else { It is unclear to me why the result from QueryAuditProofAsync indicates that the callback is not expected to be called. I suggest an additional flag to the method (or a NULL callback) to indicate this is indeed the expected behaviour (you could then add a callback created in this function to ensure it was not called)
https://codereview.chromium.org/2369373002/diff/1/components/certificate_tran... File components/certificate_transparency/log_dns_client.cc (right): https://codereview.chromium.org/2369373002/diff/1/components/certificate_tran... components/certificate_transparency/log_dns_client.cc:109: // If a query was already in progress, this will abort it first. On 2016/09/28 13:44:08, Eran Messeri wrote: > How often do you expect that case to happen? > If not at all (i.e. starting a query twice is a programming error, then DCHECK > that the next_state_ is State::NONE and then you don't have to worry about > cancelling the current_dns_transaction_. It's not the intended usage (there's little reason to reuse an AuditProofQuery) so sure, I'll just DCHECK on that. https://codereview.chromium.org/2369373002/diff/1/components/certificate_tran... components/certificate_transparency/log_dns_client.cc:176: if (net_error != net::ERR_IO_PENDING) { On 2016/09/28 13:44:08, Eran Messeri wrote: > Nit: Document why not doing anything in case of net_error being ERR_IO_PENDING > is OK. Done. https://codereview.chromium.org/2369373002/diff/1/components/certificate_tran... components/certificate_transparency/log_dns_client.cc:196: net::DnsTransactionFactory::CallbackType transaction_callback = On 2016/09/28 13:44:08, Eran Messeri wrote: > Inline transaction_callback creation & use into CreateTransaction call. Done. https://codereview.chromium.org/2369373002/diff/1/components/certificate_tran... components/certificate_transparency/log_dns_client.cc:211: // If we receive no response but no net::error either (shouldn't happen), On 2016/09/28 13:44:08, Eran Messeri wrote: > "shouldn't happen" implies this is a programming error, right? > If so, it seems a DCHECK for this case is more appropriate. Done. https://codereview.chromium.org/2369373002/diff/1/components/certificate_tran... components/certificate_transparency/log_dns_client.cc:269: net::DnsTransactionFactory::CallbackType transaction_callback = On 2016/09/28 13:44:08, Eran Messeri wrote: > Ditto, inline callback creation. Done. https://codereview.chromium.org/2369373002/diff/1/components/certificate_tran... File components/certificate_transparency/log_dns_client.h (right): https://codereview.chromium.org/2369373002/diff/1/components/certificate_tran... components/certificate_transparency/log_dns_client.h:70: // The |callback| is invoked when the query is complete, or an error occurs. On 2016/09/28 13:44:08, Eran Messeri wrote: > Document the return value - that on successful submission ERR_IO_PENDING will > be returned, another error code to indicate throttling (and everything else is a > hard-fail). Done. https://codereview.chromium.org/2369373002/diff/1/components/certificate_tran... components/certificate_transparency/log_dns_client.h:103: // efficient. On 2016/09/28 13:44:08, Eran Messeri wrote: > Be more explicit in the comment: > // A FIFO queue of queries. Since entries will always be appended to the > // end of the queue and lookups will yield entries at the beginning, > // std::list is the efficient choice. Done. https://codereview.chromium.org/2369373002/diff/1/components/certificate_tran... File components/certificate_transparency/log_dns_client_unittest.cc (right): https://codereview.chromium.org/2369373002/diff/1/components/certificate_tran... components/certificate_transparency/log_dns_client_unittest.cc:182: } else { On 2016/09/28 13:44:08, Eran Messeri wrote: > It is unclear to me why the result from QueryAuditProofAsync indicates that the > callback is not expected to be called. > > I suggest an additional flag to the method (or a NULL callback) to indicate this > is indeed the expected behaviour (you could then add a callback created in this > function to ensure it was not called) If LogDnsClient::QueryAuditProof returns ERR_IO_PENDING, it's declaring that it's continuing asynchronously and the callback will be invoked. Any other net::Error indicates otherwise, so there's so point waiting for the callback. It doesn't matter if the test is expecting it to be called because the test will fail immediately due to an unexpected return value.
The CQ bit was checked by robpercival@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm % a few comments, mostly regarding the tests. https://codereview.chromium.org/2369373002/diff/60001/components/certificate_... File components/certificate_transparency/log_dns_client.cc (right): https://codereview.chromium.org/2369373002/diff/60001/components/certificate_... components/certificate_transparency/log_dns_client.cc:113: // The leaf hash and callback will be used later. nit: Explain when later is. https://codereview.chromium.org/2369373002/diff/60001/components/certificate_... components/certificate_transparency/log_dns_client.cc:167: // is kept alive in |current_dns_transaction_|. Would it be appropriate , then, to add a DCHECK_EQ(current_dns_transaction_, transaction)? https://codereview.chromium.org/2369373002/diff/60001/components/certificate_... components/certificate_transparency/log_dns_client.cc:293: net::DnsTransactionFactory::CallbackType transaction_callback = inline transaction_callback creation/use. https://codereview.chromium.org/2369373002/diff/60001/components/certificate_... File components/certificate_transparency/log_dns_client.h (right): https://codereview.chromium.org/2369373002/diff/60001/components/certificate_... components/certificate_transparency/log_dns_client.h:72: // The |callback| is invoked when the query is complete, or an asynchronous Nit: Mention that the |callback| is invoked only if net::ERR_IO_PENDING was returned. https://codereview.chromium.org/2369373002/diff/60001/components/certificate_... components/certificate_transparency/log_dns_client.h:77: // - net::ERR_TEMPORARILY_THROTTLED if the maximum number of concurrent nit: Add a TODO to refer to the mechanism for getting notifications of un-throttling (entirely optional; as a way of documenting our intent to provide one) https://codereview.chromium.org/2369373002/diff/60001/components/certificate_... File components/certificate_transparency/log_dns_client_unittest.cc (right): https://codereview.chromium.org/2369373002/diff/60001/components/certificate_... components/certificate_transparency/log_dns_client_unittest.cc:129: EXPECT_TRUE(!called_) << "Callback invoked more than once"; nit: EXPECT_FALSE ? https://codereview.chromium.org/2369373002/diff/60001/components/certificate_... components/certificate_transparency/log_dns_client_unittest.cc:163: net::Error QueryAuditProofAsync( Seems like this method is not justified anymore, as it's simply calling LogDnsClient::QueryAuditProof https://codereview.chromium.org/2369373002/diff/60001/components/certificate_... components/certificate_transparency/log_dns_client_unittest.cc:785: callback2.WaitUntilRun(TestTimeouts::tiny_timeout()); Nit: Would it make sense to move that to line 795, so that all the assertions/expectations are checked against callback1? Would make the test fail slightly faster if the outcome from callback1 isn't as expected.
Patchset #5 (id:80001) has been deleted
The CQ bit was checked by robpercival@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
PTAL https://codereview.chromium.org/2369373002/diff/60001/components/certificate_... File components/certificate_transparency/log_dns_client.cc (right): https://codereview.chromium.org/2369373002/diff/60001/components/certificate_... components/certificate_transparency/log_dns_client.cc:113: // The leaf hash and callback will be used later. On 2016/09/30 09:26:51, Eran Messeri wrote: > nit: Explain when later is. Done. https://codereview.chromium.org/2369373002/diff/60001/components/certificate_... components/certificate_transparency/log_dns_client.cc:167: // is kept alive in |current_dns_transaction_|. On 2016/09/30 09:26:51, Eran Messeri wrote: > Would it be appropriate , then, to add a DCHECK_EQ(current_dns_transaction_, > transaction)? Done. https://codereview.chromium.org/2369373002/diff/60001/components/certificate_... components/certificate_transparency/log_dns_client.cc:293: net::DnsTransactionFactory::CallbackType transaction_callback = On 2016/09/30 09:26:51, Eran Messeri wrote: > inline transaction_callback creation/use. Done. https://codereview.chromium.org/2369373002/diff/60001/components/certificate_... File components/certificate_transparency/log_dns_client.h (right): https://codereview.chromium.org/2369373002/diff/60001/components/certificate_... components/certificate_transparency/log_dns_client.h:72: // The |callback| is invoked when the query is complete, or an asynchronous On 2016/09/30 09:26:51, Eran Messeri wrote: > Nit: Mention that the |callback| is invoked only if net::ERR_IO_PENDING was > returned. Done. https://codereview.chromium.org/2369373002/diff/60001/components/certificate_... components/certificate_transparency/log_dns_client.h:77: // - net::ERR_TEMPORARILY_THROTTLED if the maximum number of concurrent On 2016/09/30 09:26:51, Eran Messeri wrote: > nit: Add a TODO to refer to the mechanism for getting notifications of > un-throttling (entirely optional; as a way of documenting our intent to provide > one) Done. https://codereview.chromium.org/2369373002/diff/60001/components/certificate_... File components/certificate_transparency/log_dns_client_unittest.cc (right): https://codereview.chromium.org/2369373002/diff/60001/components/certificate_... components/certificate_transparency/log_dns_client_unittest.cc:129: EXPECT_TRUE(!called_) << "Callback invoked more than once"; On 2016/09/30 09:26:51, Eran Messeri wrote: > nit: EXPECT_FALSE ? Done. https://codereview.chromium.org/2369373002/diff/60001/components/certificate_... components/certificate_transparency/log_dns_client_unittest.cc:163: net::Error QueryAuditProofAsync( On 2016/09/30 09:26:52, Eran Messeri wrote: > Seems like this method is not justified anymore, as it's simply calling > LogDnsClient::QueryAuditProof Done. https://codereview.chromium.org/2369373002/diff/60001/components/certificate_... components/certificate_transparency/log_dns_client_unittest.cc:785: callback2.WaitUntilRun(TestTimeouts::tiny_timeout()); On 2016/09/30 09:26:52, Eran Messeri wrote: > Nit: Would it make sense to move that to line 795, so that all the > assertions/expectations are checked against callback1? Would make the test fail > slightly faster if the outcome from callback1 isn't as expected. Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: 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_...)
https://codereview.chromium.org/2369373002/diff/120001/components/certificate... File components/certificate_transparency/log_dns_client.cc (right): https://codereview.chromium.org/2369373002/diff/120001/components/certificate... components/certificate_transparency/log_dns_client.cc:93: AuditProofQuery(net::DnsClient* dns_client, The choice to integrate both the definition and the declaration within the same makes it hard to do a structured read of the code for comprehension - that is, starting with the header and observing the contracts, and then looking at the implementation. I would encourage you to consider splitting the two, which can help provide what is desired - a logical overview of execution without bogging down with the implementation details, and then the implementation details (in the same order) https://codereview.chromium.org/2369373002/diff/120001/components/certificate... components/certificate_transparency/log_dns_client.cc:111: // Start a new audit proof. I appreciate the commenting, but this one seems overly specific :) https://codereview.chromium.org/2369373002/diff/120001/components/certificate... components/certificate_transparency/log_dns_client.cc:113: // The leaf hash will be used during the REQUEST_LEAF_INDEX state. I appreciate the commenting, but this one seems overly specific :) It's documenting code "elsewhere" https://codereview.chromium.org/2369373002/diff/120001/components/certificate... components/certificate_transparency/log_dns_client.cc:116: // state. I appreciate the commenting, but this one seems overly specific :) It's documenting code "elsewhere" https://codereview.chromium.org/2369373002/diff/120001/components/certificate... components/certificate_transparency/log_dns_client.cc:132: enum class State { I believe this may be the first use of enum class here for the state machine, and I'm not sure it meaningfully helps readability (or writability) Historically, this has been accomplished by an unnamed enum. https://codereview.chromium.org/2369373002/diff/120001/components/certificate... components/certificate_transparency/log_dns_client.cc:173: DCHECK_EQ(current_dns_transaction_.get(), transaction); Is the .get() necessary? operator== is supposed to have sufficient overloads for this equality check. https://codereview.chromium.org/2369373002/diff/120001/components/certificate... components/certificate_transparency/log_dns_client.cc:182: FROM_HERE, base::Bind(callback_, result, base::Unretained(this))); Why do you PostTask yield here? This is generally an anti-pattern, *especially* with the base::Unretained(this) - I cannot convince myself this memory pattern is safe. https://codereview.chromium.org/2369373002/diff/120001/components/certificate... components/certificate_transparency/log_dns_client.cc:244: // to rate-limit DNS requests. This last comment is not obvious about why this is. Could you explain a little more in this code review? https://codereview.chromium.org/2369373002/diff/120001/components/certificate... components/certificate_transparency/log_dns_client.cc:248: proof_->leaf_index, tree_size_)); Are these DCHECKs guarding against programmer error? Or malformed input? It looks like malformed input. https://codereview.chromium.org/2369373002/diff/120001/components/certificate... components/certificate_transparency/log_dns_client.cc:282: // If we don't have all of the proof nodes yet, request more. // Keep requesting more proof nodes until all of them are received. https://groups.google.com/a/chromium.org/d/topic/chromium-dev/NH-S6KCkr2M/dis... https://codereview.chromium.org/2369373002/diff/120001/components/certificate... File components/certificate_transparency/log_dns_client.h (right): https://codereview.chromium.org/2369373002/diff/120001/components/certificate... components/certificate_transparency/log_dns_client.h:86: net::Error QueryAuditProof(base::StringPiece domain_for_log, 1) It's exceptionally rare to see a net::Error return here, such that I'm not sure this is the idiomatic pattern. It also prevents the net::CompletionCallback pattern throughout (combined with the bits below). I realize the argument here is that int is a less-specific type, but it does bear calling out here that the number of places that build their contract around net::Error are quite rare. 2) This doesn't support a true sync/async contract, in that the API usage explicitly prohibits you from synchronously returning an answer, because you're implicitly stating that all successful queries will be asynchronous. As such, it doesn't support the continuation-style, because your entrance will be different if you had a sync return (just a net::Error) or an async return (a net::error and a MerkleAuditProof) I tried to mention example cases such as HostResolver or CertVerifier for "sync/async with extra values" (which also yields itself nicely to the state embodiment pattern, via the opaque Request object) https://codereview.chromium.org/2369373002/diff/120001/components/certificate... File components/certificate_transparency/log_dns_client_unittest.cc (right): https://codereview.chromium.org/2369373002/diff/120001/components/certificate... components/certificate_transparency/log_dns_client_unittest.cc:180: } This (the else condition) is really hard to wrap my head around as correct. Is it necessary? Is it trying to do too much here? https://codereview.chromium.org/2369373002/diff/120001/components/certificate... components/certificate_transparency/log_dns_client_unittest.cc:782: callback2.WaitUntilRun(TestTimeouts::tiny_timeout()); Why do you do tiny_timeout here, but a 0 timeout in the above case of "callback shouldn't be run". What's the significance here, if any?
https://codereview.chromium.org/2369373002/diff/120001/components/certificate... File components/certificate_transparency/log_dns_client.cc (right): https://codereview.chromium.org/2369373002/diff/120001/components/certificate... components/certificate_transparency/log_dns_client.cc:93: AuditProofQuery(net::DnsClient* dns_client, On 2016/10/03 23:38:47, Ryan Sleevi (slow) wrote: > The choice to integrate both the definition and the declaration within the same > makes it hard to do a structured read of the code for comprehension - that is, > starting with the header and observing the contracts, and then looking at the > implementation. > > I would encourage you to consider splitting the two, which can help provide what > is desired - a logical overview of execution without bogging down with the > implementation details, and then the implementation details (in the same order) Done. https://codereview.chromium.org/2369373002/diff/120001/components/certificate... components/certificate_transparency/log_dns_client.cc:111: // Start a new audit proof. On 2016/10/03 23:38:47, Ryan Sleevi (slow) wrote: > I appreciate the commenting, but this one seems overly specific :) Done. https://codereview.chromium.org/2369373002/diff/120001/components/certificate... components/certificate_transparency/log_dns_client.cc:113: // The leaf hash will be used during the REQUEST_LEAF_INDEX state. On 2016/10/03 23:38:47, Ryan Sleevi (slow) wrote: > I appreciate the commenting, but this one seems overly specific :) It's > documenting code "elsewhere" Done. https://codereview.chromium.org/2369373002/diff/120001/components/certificate... components/certificate_transparency/log_dns_client.cc:116: // state. On 2016/10/03 23:38:47, Ryan Sleevi (slow) wrote: > I appreciate the commenting, but this one seems overly specific :) It's > documenting code "elsewhere" Done. https://codereview.chromium.org/2369373002/diff/120001/components/certificate... components/certificate_transparency/log_dns_client.cc:132: enum class State { On 2016/10/03 23:38:47, Ryan Sleevi (slow) wrote: > I believe this may be the first use of enum class here for the state machine, > and I'm not sure it meaningfully helps readability (or writability) > > Historically, this has been accomplished by an unnamed enum. Looks like it's been done a few times before (https://cs.chromium.org/search/?q="enum+class+State"), but I'm fine with changing it if you think the historic way of doing it is better. Would you prefix the enums with "STATE_" in that case? https://codereview.chromium.org/2369373002/diff/120001/components/certificate... components/certificate_transparency/log_dns_client.cc:173: DCHECK_EQ(current_dns_transaction_.get(), transaction); On 2016/10/03 23:38:47, Ryan Sleevi (slow) wrote: > Is the .get() necessary? operator== is supposed to have sufficient overloads for > this equality check. Yes, as unique_ptr doesn't have an operator== overload for raw pointers (see http://en.cppreference.com/w/cpp/memory/unique_ptr/operator_cmp). https://codereview.chromium.org/2369373002/diff/120001/components/certificate... components/certificate_transparency/log_dns_client.cc:182: FROM_HERE, base::Bind(callback_, result, base::Unretained(this))); On 2016/10/03 23:38:47, Ryan Sleevi (slow) wrote: > Why do you PostTask yield here? This is generally an anti-pattern, *especially* > with the base::Unretained(this) - I cannot convince myself this memory pattern > is safe. It's safe because the callback will only be invoked if LogDnsClient is still alive, which in turn guarantees that the AuditProofQuery is alive (it only deletes them in the callback). I could safely invoke callback_ synchronously though I think, so lets do that instead. https://codereview.chromium.org/2369373002/diff/120001/components/certificate... components/certificate_transparency/log_dns_client.cc:244: // to rate-limit DNS requests. On 2016/10/03 23:38:47, Ryan Sleevi (slow) wrote: > This last comment is not obvious about why this is. Could you explain a little > more in this code review? If AuditProofQuery could perform parallel DNS requests as described above, there'd have to be something like a shared request counter amongst all AuditProofQueries that allow them to rate-limit themselves. LogDnsClient would no longer be able to perform rate-limiting simply at the beginning of a query, as it does now. https://codereview.chromium.org/2369373002/diff/120001/components/certificate... components/certificate_transparency/log_dns_client.cc:248: proof_->leaf_index, tree_size_)); On 2016/10/03 23:38:47, Ryan Sleevi (slow) wrote: > Are these DCHECKs guarding against programmer error? Or malformed input? It > looks like malformed input. These variables are validated as soon as they are extracted from a DNS message, and this is just asserting pre-conditions that could only be violated if a bug is introduced. Still, given that these are network inputs, perhaps double-checking them and returning a net::Error is warranted. https://codereview.chromium.org/2369373002/diff/120001/components/certificate... components/certificate_transparency/log_dns_client.cc:282: // If we don't have all of the proof nodes yet, request more. On 2016/10/03 23:38:47, Ryan Sleevi (slow) wrote: > // Keep requesting more proof nodes until all of them are received. > > https://groups.google.com/a/chromium.org/d/topic/chromium-dev/NH-S6KCkr2M/dis... Done. https://codereview.chromium.org/2369373002/diff/120001/components/certificate... File components/certificate_transparency/log_dns_client.h (right): https://codereview.chromium.org/2369373002/diff/120001/components/certificate... components/certificate_transparency/log_dns_client.h:86: net::Error QueryAuditProof(base::StringPiece domain_for_log, On 2016/10/03 23:38:47, Ryan Sleevi (slow) wrote: > 1) It's exceptionally rare to see a net::Error return here, such that I'm not > sure this is the idiomatic pattern. It also prevents the net::CompletionCallback > pattern throughout (combined with the bits below). I realize the argument here > is that int is a less-specific type, but it does bear calling out here that the > number of places that build their contract around net::Error are quite rare. > > 2) This doesn't support a true sync/async contract, in that the API usage > explicitly prohibits you from synchronously returning an answer, because you're > implicitly stating that all successful queries will be asynchronous. As such, it > doesn't support the continuation-style, because your entrance will be different > if you had a sync return (just a net::Error) or an async return (a net::error > and a MerkleAuditProof) > > I tried to mention example cases such as HostResolver or CertVerifier for > "sync/async with extra values" (which also yields itself nicely to the state > embodiment pattern, via the opaque Request object) I'll look at making this emulate HostResolver much more closely. https://codereview.chromium.org/2369373002/diff/120001/components/certificate... File components/certificate_transparency/log_dns_client_unittest.cc (right): https://codereview.chromium.org/2369373002/diff/120001/components/certificate... components/certificate_transparency/log_dns_client_unittest.cc:180: } On 2016/10/03 23:38:47, Ryan Sleevi (slow) wrote: > This (the else condition) is really hard to wrap my head around as correct. Is > it necessary? Is it trying to do too much here? Without it, tests wouldn't detect when a callback is invoked even though QueryAuditProof didn't return ERR_IO_PENDING. If we don't care about covering that eventuality, I can remove the else condition. https://codereview.chromium.org/2369373002/diff/120001/components/certificate... components/certificate_transparency/log_dns_client_unittest.cc:782: callback2.WaitUntilRun(TestTimeouts::tiny_timeout()); On 2016/10/03 23:38:47, Ryan Sleevi (slow) wrote: > Why do you do tiny_timeout here, but a 0 timeout in the above case of "callback > shouldn't be run". What's the significance here, if any? Good point, this should be the same as in the other case. The only difference between the two is that this extends the test duration by 100ms.
Patchset #7 (id:140001) has been deleted
The CQ bit was checked by robpercival@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromeos_x86-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_x86-ge...) linux_chromium_chromeos_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by robpercival@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Patchset #9 (id:200001) has been deleted
The CQ bit was checked by robpercival@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by robpercival@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
LGTM as is, with a few nits. I can buy the State enum change as is https://codereview.chromium.org/2369373002/diff/240001/components/certificate... File components/certificate_transparency/log_dns_client.cc (right): https://codereview.chromium.org/2369373002/diff/240001/components/certificate... components/certificate_transparency/log_dns_client.cc:107: base::Callback<void(net::Error result, AuditProofQuery* query)>; Same remarks on the other CL - I don't think you need to explicitly force |this| to be bound, since the caller can/should curry it (which can then make more explicit guarantees about the lifetime/ownership semantics) That is, just using net::CompletionCallback here, and having the LogDnsClient base::Bind() the Query https://codereview.chromium.org/2369373002/diff/240001/components/certificate... components/certificate_transparency/log_dns_client.cc:373: if (factory == nullptr) { nit: if (!factory) - while mostly semantic/pedantic, it lends itself easier because it's a consistent checking method for all of our pointer types (scoped_refptr, std::unique_ptr<>, base pointers). However, this may not be the case anymore with C++11 having a nullptr_t<> type, in that I know at least unique_ptr<> has nullptr_t comparison overrides, but I don't believe we added those for scoped_refptr<>. But it's also shorter to write and still semantically equivalent. But this is a REALLY pedantic nit that previous //base OWNERs had dinged me on. https://codereview.chromium.org/2369373002/diff/240001/components/certificate... components/certificate_transparency/log_dns_client.cc:432: return query->Start(leaf_hash, internal_callback); return query->Start(leaf_hash, base::Bind(&LogDnsClient::QueryAuditProofComplete, weak_ptr_factory_.GetWeakPtr(), callback, query));
Patchset #12 (id:280001) has been deleted
The CQ bit was checked by robpercival@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2369373002/diff/120001/components/certificate... File components/certificate_transparency/log_dns_client.h (right): https://codereview.chromium.org/2369373002/diff/120001/components/certificate... components/certificate_transparency/log_dns_client.h:86: net::Error QueryAuditProof(base::StringPiece domain_for_log, On 2016/10/04 18:35:40, Rob Percival wrote: > On 2016/10/03 23:38:47, Ryan Sleevi (slow) wrote: > > 1) It's exceptionally rare to see a net::Error return here, such that I'm not > > sure this is the idiomatic pattern. It also prevents the > net::CompletionCallback > > pattern throughout (combined with the bits below). I realize the argument here > > is that int is a less-specific type, but it does bear calling out here that > the > > number of places that build their contract around net::Error are quite rare. > > > > 2) This doesn't support a true sync/async contract, in that the API usage > > explicitly prohibits you from synchronously returning an answer, because > you're > > implicitly stating that all successful queries will be asynchronous. As such, > it > > doesn't support the continuation-style, because your entrance will be > different > > if you had a sync return (just a net::Error) or an async return (a net::error > > and a MerkleAuditProof) > > > > I tried to mention example cases such as HostResolver or CertVerifier for > > "sync/async with extra values" (which also yields itself nicely to the state > > embodiment pattern, via the opaque Request object) > > I'll look at making this emulate HostResolver much more closely. Done. https://codereview.chromium.org/2369373002/diff/240001/components/certificate... File components/certificate_transparency/log_dns_client.cc (right): https://codereview.chromium.org/2369373002/diff/240001/components/certificate... components/certificate_transparency/log_dns_client.cc:107: base::Callback<void(net::Error result, AuditProofQuery* query)>; On 2016/10/07 15:37:31, Ryan Sleevi (slow) wrote: > Same remarks on the other CL - I don't think you need to explicitly force |this| > to be bound, since the caller can/should curry it (which can then make more > explicit guarantees about the lifetime/ownership semantics) > > That is, just using net::CompletionCallback here, and having the LogDnsClient > base::Bind() the Query Done. https://codereview.chromium.org/2369373002/diff/240001/components/certificate... components/certificate_transparency/log_dns_client.cc:373: if (factory == nullptr) { On 2016/10/07 15:37:30, Ryan Sleevi (slow) wrote: > nit: if (!factory) - while mostly semantic/pedantic, it lends itself easier > because it's a consistent checking method for all of our pointer types > (scoped_refptr, std::unique_ptr<>, base pointers). > > However, this may not be the case anymore with C++11 having a nullptr_t<> type, > in that I know at least unique_ptr<> has nullptr_t comparison overrides, but I > don't believe we added those for scoped_refptr<>. But it's also shorter to write > and still semantically equivalent. > > But this is a REALLY pedantic nit that previous //base OWNERs had dinged me on. Done. https://codereview.chromium.org/2369373002/diff/240001/components/certificate... components/certificate_transparency/log_dns_client.cc:432: return query->Start(leaf_hash, internal_callback); On 2016/10/07 15:37:30, Ryan Sleevi (slow) wrote: > return query->Start(leaf_hash, > base::Bind(&LogDnsClient::QueryAuditProofComplete, > weak_ptr_factory_.GetWeakPtr(), callback, query)); Done.
The CQ bit was checked by robpercival@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Patchset #13 (id:320001) has been deleted
The CQ bit was checked by robpercival@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
still lgtm. I find the use of Variadic Templates in the test a bit unnecessary, but can't articulate a stronger argument against it. https://codereview.chromium.org/2369373002/diff/300001/components/certificate... File components/certificate_transparency/log_dns_client.cc (right): https://codereview.chromium.org/2369373002/diff/300001/components/certificate... components/certificate_transparency/log_dns_client.cc:117: net::ct::MerkleAuditProof* proof); Is the caller expected to maintain ownership of |proof|? may be worth documenting. https://codereview.chromium.org/2369373002/diff/300001/components/certificate... components/certificate_transparency/log_dns_client.cc:166: bool StartDnsTransaction(const std::string& qname); Nit: documentation. https://codereview.chromium.org/2369373002/diff/300001/components/certificate... components/certificate_transparency/log_dns_client.cc:216: leaf_hash.CopyToString(&leaf_hash_); API nit: Can the API be changed to enable moving the leaf_hash into the method? The caller (SingleTreeTracker) doesn't care about the leaf hash once the query is submitted, so it would save a string copy. https://codereview.chromium.org/2369373002/diff/300001/components/certificate... File components/certificate_transparency/log_dns_client_unittest.cc (right): https://codereview.chromium.org/2369373002/diff/300001/components/certificate... components/certificate_transparency/log_dns_client_unittest.cc:110: template <typename... Types> According to https://chromium-cpp.appspot.com/, usage of variadic templates "should be rare". It seems to me you're using it here to get around defining all the parameters of QueryAuditProof, or avoid having to change them if the signature of QueryAuditProof changes? My only argument for not using Variadic Templates here is that you don't absolutely need it, which is not a very strong one.
The CQ bit was checked by robpercival@chromium.org to run a CQ dry run
https://codereview.chromium.org/2369373002/diff/300001/components/certificate... File components/certificate_transparency/log_dns_client.cc (right): https://codereview.chromium.org/2369373002/diff/300001/components/certificate... components/certificate_transparency/log_dns_client.cc:117: net::ct::MerkleAuditProof* proof); On 2016/10/14 11:05:13, Eran Messeri wrote: > Is the caller expected to maintain ownership of |proof|? may be worth > documenting. Done. https://codereview.chromium.org/2369373002/diff/300001/components/certificate... components/certificate_transparency/log_dns_client.cc:166: bool StartDnsTransaction(const std::string& qname); On 2016/10/14 11:05:13, Eran Messeri wrote: > Nit: documentation. Done. https://codereview.chromium.org/2369373002/diff/300001/components/certificate... components/certificate_transparency/log_dns_client.cc:216: leaf_hash.CopyToString(&leaf_hash_); On 2016/10/14 11:05:13, Eran Messeri wrote: > API nit: Can the API be changed to enable moving the leaf_hash into the method? > The caller (SingleTreeTracker) doesn't care about the leaf hash once the query > is submitted, so it would save a string copy. Done. https://codereview.chromium.org/2369373002/diff/300001/components/certificate... File components/certificate_transparency/log_dns_client_unittest.cc (right): https://codereview.chromium.org/2369373002/diff/300001/components/certificate... components/certificate_transparency/log_dns_client_unittest.cc:110: template <typename... Types> On 2016/10/14 11:05:13, Eran Messeri wrote: > According to https://chromium-cpp.appspot.com/, usage of variadic templates > "should be rare". > It seems to me you're using it here to get around defining all the parameters of > QueryAuditProof, or avoid having to change them if the signature of > QueryAuditProof changes? > My only argument for not using Variadic Templates here is that you don't > absolutely need it, which is not a very strong one. That's correct. This is mostly just forwarding to log_client->QueryAuditProof, and forwarding functions are the poster child for variadic templates.
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by robpercival@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from rsleevi@chromium.org, eranm@chromium.org Link to the patchset: https://chromiumcodereview.appspot.com/2369373002/#ps360001 (title: "Addresses Eran's comments")
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.
Committed patchset #14 (id:360001)
Message was sent while issue was closed.
Description was changed from ========== LogDnsClient now returns some errors synchronously This allows users to handle issues, such as throttling, immediately. Uses a sync-async DoLoop pattern internally now. BUG=624894 ========== to ========== LogDnsClient now returns some errors synchronously This allows users to handle issues, such as throttling, immediately. Uses a sync-async DoLoop pattern internally now. BUG=624894 Committed: https://crrev.com/cfe43126e0716d7626175dd0c4cac9a12863a50b Cr-Commit-Position: refs/heads/master@{#426451} ==========
Message was sent while issue was closed.
Patchset 14 (id:??) landed as https://crrev.com/cfe43126e0716d7626175dd0c4cac9a12863a50b Cr-Commit-Position: refs/heads/master@{#426451} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
