Closed Bug 620660 Opened 14 years ago Closed 13 years ago

Ensure the disk-cache stays around until all cache-operations have finished

Categories

(Core :: Networking: Cache, defect)

defect
Not set
major

Tracking

()

RESOLVED FIXED
Tracking Status
blocking2.0 --- -

People

(Reporter: bjarne, Assigned: bjarne)

References

Details

Attachments

(1 file, 3 obsolete files)

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.
blocking2.0: --- → ?
Attached patch Proposed solution (obsolete) — Splinter Review
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)
Tryserver run looks reasonable,
Comment on attachment 499023 [details] [diff] [review]
Proposed solution

Changed reviewer - Michal seems busy.
Attachment #499023 - Flags: review?(michal.novotny) → review?(bzbarsky)
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.
(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...
Attachment #499023 - Flags: review?(bzbarsky) → review?(michal.novotny)
Blocks: 549767
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)
Attached patch Proposed solution v2 (obsolete) — Splinter Review
Updated with comments from reviewer
Attachment #499023 - Attachment is obsolete: true
Attachment #500760 - Flags: review?(michal.novotny)
Attached patch Proposed solution v3 (obsolete) — Splinter Review
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 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+
(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.
(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.
Fair enough. :)  About the Dispatch()-issue: Never mind - I overlooked the connection with the change to COMPtr.
Updated with review-comments. Requesting approval for 2.0
Attachment #500771 - Attachment is obsolete: true
Attachment #501111 - Flags: approval2.0?
Not blocking on this, but I'll approve the patch.
blocking2.0: ? → -
Attachment #501111 - Flags: approval2.0? → approval2.0+
Didn't see the approval...  Patch still applies - requesting check-in.
Status: NEW → ASSIGNED
Keywords: checkin-needed
http://hg.mozilla.org/mozilla-central/rev/22a635e15db1
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: