|
|
Created:
8 years, 1 month ago by blundell Modified:
7 years, 1 month ago CC:
chromium-reviews, cbentzel+watch_chromium.org, darin-cc_chromium.org Base URL:
http://git.chromium.org/chromium/src.git@master Visibility:
Public. |
DescriptionEnsure that OCSPIOLoop is associated with right thread if restarted.
In tests, g_ocsp_io_loop can be shut down and restarted. Ensure that if it is
restarted it is associated with the current IO thread to avoid DCHECK's
potentially going off when it is subsequently (re-)shut down.
BUG=
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=166869
Patch Set 1 #Patch Set 2 : Edits #
Total comments: 8
Patch Set 3 : Response to review #
Total comments: 4
Patch Set 4 : Response to review #
Total comments: 3
Messages
Total messages: 14 (0 generated)
Ryan, This patch is following up our discussion on IRC last week. WDYT?
Regrettably, I've since popped the stack from our IRC conversation. I recall that one of the concerns I had was regarding worker threads attempting to access the loop later on (hence the point of shutdown). I'll just want to convince myself that there's no chance of a race on |shutdown_| from workers (eg: that it's a leak check, and not a threading signal) https://chromiumcodereview.appspot.com/11347039/diff/2001/net/ocsp/nss_ocsp.cc File net/ocsp/nss_ocsp.cc (right): https://chromiumcodereview.appspot.com/11347039/diff/2001/net/ocsp/nss_ocsp.c... net/ocsp/nss_ocsp.cc:69: } Perhaps moving this into an explicit Reset() method to signal the intent to re-use this (see comments below re: |used|) https://chromiumcodereview.appspot.com/11347039/diff/2001/net/ocsp/nss_ocsp.c... net/ocsp/nss_ocsp.cc:933: DCHECK(!used); What about the fact that this is not reset? Attempting to call this in a subsequent test will trigger an issue https://chromiumcodereview.appspot.com/11347039/diff/2001/net/ocsp/nss_ocsp.c... net/ocsp/nss_ocsp.cc:935: g_ocsp_io_loop.Get().StartUsing(); I'm not sure I understand the motivation here for this change? Why would someone not call EnsureNSSHttpIOInit? (Granted, perhaps I'm confused myself on the distinction between the two)
Thanks for the review. Comments answered...no code changes as of yet, pending discussion on which direction to go in. https://chromiumcodereview.appspot.com/11347039/diff/2001/net/ocsp/nss_ocsp.cc File net/ocsp/nss_ocsp.cc (right): https://chromiumcodereview.appspot.com/11347039/diff/2001/net/ocsp/nss_ocsp.c... net/ocsp/nss_ocsp.cc:69: } I thought about factoring this out into its own method, and maybe even adding that method to the public interface so that the re-use would have to be enabled via explicit client invocation rather than happening beyond the scenes (and could make clear via comments/method naming that the re-use should be enabled only when testing). Do you like that idea? On 2012/11/05 14:43:31, Ryan Sleevi wrote: > Perhaps moving this into an explicit Reset() method to signal the intent to > re-use this (see comments below re: |used|) https://chromiumcodereview.appspot.com/11347039/diff/2001/net/ocsp/nss_ocsp.c... net/ocsp/nss_ocsp.cc:933: DCHECK(!used); used_ is set to false in Shutdown() (where shutdown_ is set to true). On 2012/11/05 14:43:31, Ryan Sleevi wrote: > What about the fact that this is not reset? > > Attempting to call this in a subsequent test will trigger an issue https://chromiumcodereview.appspot.com/11347039/diff/2001/net/ocsp/nss_ocsp.c... net/ocsp/nss_ocsp.cc:935: g_ocsp_io_loop.Get().StartUsing(); |SetMessageLoopForNSSHttpIO| is called in |IOThread::Init()|. My thought was that by adding the call to |StartUsing|, the re-use could occur transparently in the case where multiple IO threads are being created sequentially for testing. Of course, this goes directly against the idea of making the re-use explicit and targeted...I was torn on which direction to go. On 2012/11/05 14:43:31, Ryan Sleevi wrote: > I'm not sure I understand the motivation here for this change? > > Why would someone not call EnsureNSSHttpIOInit? (Granted, perhaps I'm confused > myself on the distinction between the two)
http://codereview.chromium.org/11347039/diff/2001/net/ocsp/nss_ocsp.cc File net/ocsp/nss_ocsp.cc (right): http://codereview.chromium.org/11347039/diff/2001/net/ocsp/nss_ocsp.cc#newcode69 net/ocsp/nss_ocsp.cc:69: } On 2012/11/05 15:19:02, blundell wrote: > I thought about factoring this out into its own method, and maybe even adding > that method to the public interface so that the re-use would have to be enabled > via explicit client invocation rather than happening beyond the scenes (and > could make clear via comments/method naming that the re-use should be enabled > only when testing). Do you like that idea? IMO, yeah. Makes it easier to separate out bugs-in-tests and bugs-in-impl, and the default behaviour (requiring callers to call ResetForTest[ing] ) would allow the presubmit scripts to say "O hai, you're calling a testing method from a non-unittest file" > > On 2012/11/05 14:43:31, Ryan Sleevi wrote: > > Perhaps moving this into an explicit Reset() method to signal the intent to > > re-use this (see comments below re: |used|) > http://codereview.chromium.org/11347039/diff/2001/net/ocsp/nss_ocsp.cc#newcod... net/ocsp/nss_ocsp.cc:935: g_ocsp_io_loop.Get().StartUsing(); On 2012/11/05 15:19:02, blundell wrote: > |SetMessageLoopForNSSHttpIO| is called in |IOThread::Init()|. My thought was > that by adding the call to |StartUsing|, the re-use could occur transparently in > the case where multiple IO threads are being created sequentially for testing. > Of course, this goes directly against the idea of making the re-use explicit and > targeted...I was torn on which direction to go. > I would prefer not to tightly couple these two (eg: don't call StartUsing). Otherwise, it remains wholly unclear the distinction between this and EnsureNSSHttpIOInit
Ryan, PTAL!
lgtm https://chromiumcodereview.appspot.com/11347039/diff/7001/net/ocsp/nss_ocsp.h File net/ocsp/nss_ocsp.h (right): https://chromiumcodereview.appspot.com/11347039/diff/7001/net/ocsp/nss_ocsp.h... net/ocsp/nss_ocsp.h:30: // and associate it with the current IO thread, on which this function must be s/current IO thread/current thread/ ("IO thread" is a content/ concept, not a net/ concept) https://chromiumcodereview.appspot.com/11347039/diff/7001/net/ocsp/nss_ocsp.h... net/ocsp/nss_ocsp.h:32: // sequentially created and destroyed throughout the lifetime of the app. Delete the sentence "Should be used only..."
https://chromiumcodereview.appspot.com/11347039/diff/7001/net/ocsp/nss_ocsp.h File net/ocsp/nss_ocsp.h (right): https://chromiumcodereview.appspot.com/11347039/diff/7001/net/ocsp/nss_ocsp.h... net/ocsp/nss_ocsp.h:30: // and associate it with the current IO thread, on which this function must be On 2012/11/08 14:20:11, Ryan Sleevi wrote: > s/current IO thread/current thread/ ("IO thread" is a content/ concept, not a > net/ concept) Done. https://chromiumcodereview.appspot.com/11347039/diff/7001/net/ocsp/nss_ocsp.h... net/ocsp/nss_ocsp.h:32: // sequentially created and destroyed throughout the lifetime of the app. On 2012/11/08 14:20:11, Ryan Sleevi wrote: > Delete the sentence "Should be used only..." Done.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/blundell@chromium.org/11347039/11001
Retried try job too often for step(s) content_browsertests
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/blundell@chromium.org/11347039/11001
Change committed as 166869
Message was sent while issue was closed.
https://chromiumcodereview.appspot.com/11347039/diff/11001/net/ocsp/nss_ocsp.cc File net/ocsp/nss_ocsp.cc (right): https://chromiumcodereview.appspot.com/11347039/diff/11001/net/ocsp/nss_ocsp.... net/ocsp/nss_ocsp.cc:88: thread_checker_.CalledOnValidThread(); As is, this is a no-op. You probably meant to DCHECK(thread_checker_.CalledOnValidThread())? Can you fix this? (maybe include a BUG=314505 line)
Message was sent while issue was closed.
https://codereview.chromium.org/11347039/diff/11001/net/ocsp/nss_ocsp.cc File net/ocsp/nss_ocsp.cc (right): https://codereview.chromium.org/11347039/diff/11001/net/ocsp/nss_ocsp.cc#newc... net/ocsp/nss_ocsp.cc:88: thread_checker_.CalledOnValidThread(); This call is not a no-op: it associates |thread_checker_| with the current thread after just having detached it. I could put it inside a DCHECK, but as long as |CalledOnValidThread()| is the only public way to associate a thread_checker_ with a new thread after having called |DetachFromThread()|, I'm not sure why we would enforce that CalledOnValidThread() always has to be called in the context of something that's using the returned value. On 2013/11/05 03:01:30, Nico wrote: > As is, this is a no-op. You probably meant to > DCHECK(thread_checker_.CalledOnValidThread())? Can you fix this? (maybe include > a BUG=314505 line)
Message was sent while issue was closed.
https://codereview.chromium.org/11347039/diff/11001/net/ocsp/nss_ocsp.cc File net/ocsp/nss_ocsp.cc (right): https://codereview.chromium.org/11347039/diff/11001/net/ocsp/nss_ocsp.cc#newc... net/ocsp/nss_ocsp.cc:88: thread_checker_.CalledOnValidThread(); On 2013/11/05 16:01:33, blundell wrote: > This call is not a no-op: it associates |thread_checker_| with the current > thread after just having detached it. I could put it inside a DCHECK, but as > long as |CalledOnValidThread()| is the only public way to associate a > thread_checker_ with a new thread after having called |DetachFromThread()|, I'm > not sure why we would enforce that CalledOnValidThread() always has to be called > in the context of something that's using the returned value. Aha! This seems to be only place in the codebase where CalledOnValidThread() is used like this. Maybe there could be a comment explaining this, and we could wrap the call with ignore_result()? |