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

Status: RESOLVED FIXED

(Core :: Networking: Cache, defect)

Description

[Bjarne (:bjarne)]

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.

Comment 1

[Bjarne (:bjarne)]

Attachment: Proposed solution

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.

Comment 2

[Bjarne (:bjarne)]

Tryserver run looks reasonable,

Comment 3

[Bjarne (:bjarne)]

Comment on attachment 499023 [details] [diff] [review]
Proposed solution

Changed reviewer - Michal seems busy.

Comment 4

[Boris Zbarsky [: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.

Comment 5

[Michal Novotny [:michal]]

(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...

Comment 6

[Michal Novotny [:michal]]

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

Comment 7

[Bjarne (:bjarne)]

Attachment: Proposed solution v2

Updated with comments from reviewer

Comment 8

[Bjarne (:bjarne)]

Attachment: Proposed solution v3

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.

Comment 9

[Michal Novotny [:michal]]

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.

Comment 10

[Bjarne (:bjarne)]

(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

[Johnny Stenback (:jst)]

(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.

Comment 12

[Bjarne (:bjarne)]

Fair enough. :)  About the Dispatch()-issue: Never mind - I overlooked the connection with the change to COMPtr.

Comment 13

[Bjarne (:bjarne)]

Attachment: Proposed solution v4

Updated with review-comments. Requesting approval for 2.0

Comment 14

[Johnny Stenback (:jst)]

Not blocking on this, but I'll approve the patch.

Comment 15

[Bjarne (:bjarne)]

Didn't see the approval...  Patch still applies - requesting check-in.

Comment 16

[Patrick Walton (:pcwalton)]

http://hg.mozilla.org/mozilla-central/rev/22a635e15db1