Closed
Bug 620660
Opened 14 years ago
Closed 14 years ago
Ensure the disk-cache stays around until all cache-operations have finished
Categories
(Core :: Networking: Cache, defect)
Core
Networking: Cache
Tracking
()
RESOLVED
FIXED
Tracking | Status | |
---|---|---|
blocking2.0 | --- | - |
People
(Reporter: bjarne, Assigned: bjarne)
References
Details
Attachments
(1 file, 3 obsolete files)
(deleted),
patch
|
jst
:
approval2.0+
|
Details | Diff | Splinter Review |
Recent improvements to the cache introducing a separate cache-io thread have exposed an issue when shutting down the cache (or the profile), namely that internal data-structures of the disk-cache device (effectively the device itself) may be cleared/reset on the main-thread before all pending cache-events have finished. I see this clearly and consistently when working on bug #549767 (async deactivate/flush) because deactivating entries are done as part of the shutdown, but I believe this fundamental issue may cause problems from time to time in general (in particular I suspect it to cause bug #614513, but potentially also others). The same problem may also be present in other cache-devices and in the cache-service itself, but it is clearly seen with the disk-cache. IMO this is a pretty obvious thing to fix, and it is independent of bug #549767 which is why I'd prefer to have a separate bug for it.
Assignee | ||
Updated•14 years ago
|
blocking2.0: --- → ?
Assignee | ||
Comment 1•14 years ago
|
||
This is an excerpt from the WIP patch for bug #549767 which I believe will fix the issue described in this bug. The idea is simple: wait for all pending cache-operations to finish before doing anything harmful, at the right places. It survives simple local testing as well as Michals trick to reproduce bug #614513. Passed to tryserver.
Assignee: nobody → bjarne
Attachment #499023 -
Flags: review?(michal.novotny)
Assignee | ||
Comment 3•14 years ago
|
||
Comment on attachment 499023 [details] [diff] [review] Proposed solution Changed reviewer - Michal seems busy.
Attachment #499023 -
Flags: review?(michal.novotny) → review?(bzbarsky)
![]() |
||
Comment 4•14 years ago
|
||
I'm not sure I'm competent to review this; at least not if you want anything like a quick review. Who're the patch author and reviewer for the existing code? Seems like they would both be better reviewers.
Comment 5•14 years ago
|
||
(In reply to comment #3) > Comment on attachment 499023 [details] [diff] [review] > Proposed solution > > Changed reviewer - Michal seems busy. I could do the review soon since I've already seen the patch when we were discussing #614513...
Updated•14 years ago
|
Attachment #499023 -
Flags: review?(bzbarsky) → review?(michal.novotny)
Comment 6•14 years ago
|
||
Comment on attachment 499023 [details] [diff] [review] Proposed solution > + NS_IMETHOD Run() > + { > + nsCacheServiceAutoLock lock; // probably not needed IMO you really don't need it. Also you don't need the mIsFinished variable. > + mozilla::Monitor mMonitor; I would prefer to move the monitor into the nsBlockOnCacheThreadEvent class. > + nsCacheServiceAutoLock lock; // probably not needed > + mozilla::MonitorAutoEnter autoMonitor(nsCacheService::gService->mMonitor); > + > +#ifdef PR_LOGGING > + nsIThread *thread = NS_GetCurrentThread(); > + CACHE_LOG_DEBUG(("nsBlockOnCacheThreadEvent [%p] running on thread %x\n", this, thread)); > +#endif 80 chars please
Attachment #499023 -
Flags: review?(michal.novotny)
Assignee | ||
Comment 7•14 years ago
|
||
Updated with comments from reviewer
Attachment #499023 -
Attachment is obsolete: true
Attachment #500760 -
Flags: review?(michal.novotny)
Assignee | ||
Comment 8•14 years ago
|
||
Re-introduced the monitor in nsCacheService because holding it in the event itself seems to cause problems when the runnable it deleted by the thread executing it.
Attachment #500760 -
Attachment is obsolete: true
Attachment #500771 -
Flags: review?(michal.novotny)
Attachment #500760 -
Flags: review?(michal.novotny)
Comment 9•14 years ago
|
||
Comment on attachment 500771 [details] [diff] [review] Proposed solution v3 > + nsBlockOnCacheThreadEvent *event = new nsBlockOnCacheThreadEvent(); > + if (!event) { > + NS_WARNING("Failed creating event!"); > + return NS_ERROR_OUT_OF_MEMORY; > + } You don't have to check the return value of new. It either successfully allocates memory, or it aborts. > + nsresult rv = > + gService->mCacheIOThread->Dispatch(event, NS_DISPATCH_NORMAL); > + if (NS_FAILED(rv)) { > + NS_WARNING("Failed dispatching block-event"); > + delete event; > + return NS_ERROR_UNEXPECTED; > + } This isn't IMO quite correct. Dispatch() internally addrefs and releases the event. So in case event dispatching fails, it could be deleted there. You should do: nsCOMPtr<nsIRunnable> event = new nsBlockOnCacheThreadEvent(); Otherwise it looks good.
Attachment #500771 -
Flags: review?(michal.novotny) → review+
Assignee | ||
Comment 10•14 years ago
|
||
(In reply to comment #9) > You don't have to check the return value of new. It either successfully > allocates memory, or it aborts. But such checking is surely done all over the place...? :) See e.g. creation of mTransaction in nsHttpChannel::SetupTransaction(), mResponseHead in nsHtttpChannel::CheckCache() or mObserver in nsCacheService::Init() I assume this pattern is used for good reasons? > This isn't IMO quite correct. Dispatch() internally addrefs and releases the > event. So in case event dispatching fails, it could be deleted there. Could you point me to the code which does this book-keeping? I cannot find it.
Comment 11•14 years ago
|
||
(In reply to comment #10) > (In reply to comment #9) > > You don't have to check the return value of new. It either successfully > > allocates memory, or it aborts. > > But such checking is surely done all over the place...? :) Yes, it's only in the somewhat recent months that new has been infallible, so we still have tons of code that still checks. But there's no reason to add more, Michal is right here, you don't need checks like that any more.
Assignee | ||
Comment 12•14 years ago
|
||
Fair enough. :) About the Dispatch()-issue: Never mind - I overlooked the connection with the change to COMPtr.
Assignee | ||
Comment 13•14 years ago
|
||
Updated with review-comments. Requesting approval for 2.0
Attachment #500771 -
Attachment is obsolete: true
Attachment #501111 -
Flags: approval2.0?
Updated•14 years ago
|
Attachment #501111 -
Flags: approval2.0? → approval2.0+
Assignee | ||
Comment 15•14 years ago
|
||
Didn't see the approval... Patch still applies - requesting check-in.
Status: NEW → ASSIGNED
Keywords: checkin-needed
You need to log in
before you can comment on or make changes to this bug.
Description
•