976866 - HTTP cache v2: make sure that sequence of open, evict, open behaves as expected

Status: RESOLVED FIXED

(Core :: Networking: Cache, defect)

Description

[Honza Bambas (:mayhemer)]

Bug 968106 implements eviction of the whole cache.  We also need to evict specific stuff (new bug needed).  

We have two event queue levels of open - normal and priority.  Eviction is posted to the priority level.  However, the sequence of OPEN_NORMAL, EvictAll can turn the open result to be unexpectedly a failure or a no-content (be executed after the eviction).

So, we need to coalesce all openings to only a single level (the priority one, note: this will also include dooms) and put the evict event as the last on the open-priority level.  All atomically.

I plan a new method DispatchEviction(nsIRunnable) on the cache IO thread that will take care of all of this.

Somewhat loosely blocking bug 968106.

Comment 1

[Honza Bambas (:mayhemer)]

Attachment: v1

- new FlushOpensAndDispatch method that just appends all that is pending on OPEN level to the OPEN_PRIORITY level and then appends the given aRunnable that dooms/clears disk content ; this way cannot happen we clear something sooner then it has a chance to open
- used only in CacheFileIOManager::DoomFileByKey when I think it is currently the only place where it's very appropriate to use it
- I cannot write an automated test :(  such a test is dependent on bug 968106 and some synchronization with the cache background IO notification

Comment 2

[Michal Novotny [:michal]]

Comment on attachment 8386097 [details] [diff] [review]
v1

Review of attachment 8386097 [details] [diff] [review]:
-----------------------------------------------------------------

r+ with following fixed

(In reply to Honza Bambas (:mayhemer) from comment #1)
> - used only in CacheFileIOManager::DoomFileByKey when I think it is
> currently the only place where it's very appropriate to use it

Now it also has to be used in CacheFileIOManager::EvictAll.

::: netwerk/cache2/CacheIOThread.h
@@ +48,5 @@
>    nsresult Dispatch(nsIRunnable* aRunnable, uint32_t aLevel);
> +  // Makes sure that any previously posted event to OPEN or OPEN_PRIORITY
> +  // levels (such as file opennings and dooms) are executed before aRunnable
> +  // that is intended to evict stuff from the cache.
> +  nsresult FlushOpensAndDispatch(nsIRunnable* aRunnable);

FlushOpenEventsAndDispatch would be IMO better name, but it is not entirely correct. By "flush event" I would understand (maybe incorrectly) that the event is executed, but the open events are not executed but just reordered before the new event is dispatched. So maybe something like "ScheduleAfterLastOpenEvent" would be more correct.

Comment 3

[Honza Bambas (:mayhemer)]

Attachment: v1.1 [to land]

Since I wanted to keep the work Dispatch in the method name, I've chosen DispatchAfterPendingOpenes.

Comment 4

[Honza Bambas (:mayhemer)]

https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?changeset=6ba86b56023e

Comment 5

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/eea0fc4cde4b