Closed Bug 976866 Opened 6 years ago Closed 6 years ago

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

Categories

(Core :: Networking: Cache, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla31
Tracking Status
firefox31 --- fixed

People

(Reporter: mayhemer, Assigned: mayhemer)

References

Details

(Whiteboard: [qa-])

Attachments

(1 file, 1 obsolete file)

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.
Attached patch v1 (obsolete) — Splinter Review
- 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
Attachment #8386097 - Flags: review?(michal.novotny)
Blocks: 977766
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.
Attachment #8386097 - Flags: review?(michal.novotny) → review+
Attached patch v1.1 [to land]Splinter Review
Since I wanted to keep the work Dispatch in the method name, I've chosen DispatchAfterPendingOpenes.
Attachment #8386097 - Attachment is obsolete: true
Attachment #8400809 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/eea0fc4cde4b
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla31
Whiteboard: [qa-]
You need to log in before you can comment on or make changes to this bug.