922741 - HTTP cache v2: make callbacks iteration in CacheEntry smarter

Status: RESOLVED FIXED

(Core :: Networking: Cache, defect)

Description

[Honza Bambas (:mayhemer)]

- improve threading
- improve flagging (needs ENTRY_WANTED_COMPLETE, better code for r/o callbacks)

Comment 1

[Honza Bambas (:mayhemer)]

Attachment: v1

This patch is base for two goals:
* fix concurrent read of non-resumable responses (leads to broken pages etc.)
* adaption of wyciwyg channels to the new cache api


api changes:

- new onCacheEntryCheck result: ENTRY_WANTED_COMPLETE, see bellow
- new open flag: nsICacheStorage::CHECK_MULTITHREADED, see bellow


cache2 code changes (disabled by default):

- the callbacks array (only a single one now) in CacheEntry now keeps a helper class that:
  - refers the callback interface
  - records the consumer's demand for read-only open
  - whether it's OK to call OnCacheEntryCheck on any thread (the CHECK_M.T. flag)
  - remember on which thread to invoke OnCacheEntryAvail (and OnCacheEntryCheck when not multithreaded) callback(s) ; this is the thread where AsyncOpenURI has been called
  - whether the consumer wants onAvail be called only after the output stream is closed (the WANT_COMPLETE result of onCheck) ; needed for non-resumable responses to prevent concurrent reading
  - whether the consumer wants the entry at all (NOT_WANTED result)
- mainThreadOnly r/o attribute removed from nsICacheEntryOpenCallback, CHECK_MULTITHREADED does the job now
- CacheFileOutputStream refers a new small helper class CacheOutputCloseListener (that refers the CacheEntry) and notifies this listener when the stream has been closed (removed from CacheFile) ; CacheEntry then invokes callbacks waiting for the complete entry
- small CacheEntry::mFile cleanup
- other very small cleanups
- test for ENTRY_WANTED_COMPLETE


cache1 code changes, need careful review:

- calling asyncOpenURI is now allowed on the main thread and on the cache thread
- when OPEN_TRUNCATE (-> access = WRITE only) we call sync OpenCacheEntry ; this happens only on the cache thread
- when asyncOpenURI is called on the cache thread, processing the open is done synchronously (no repost to the cache thread)


https://tbpl.mozilla.org/?tree=Try&rev=2931fc6bbb9d

Comment 2

[Honza Bambas (:mayhemer)]

I have an updated patch already.  Will submit tomorrow.

Comment 3

[Honza Bambas (:mayhemer)]

(In reply to Honza Bambas (:mayhemer) from comment #2)
> I have an updated patch already.  Will submit tomorrow.

Hmm.. there are just merge updates.  The patch has not changed.  Feel free to review this version.

(Too tired..)

Comment 4

[Honza Bambas (:mayhemer)]

Attachment: v1

Comment 5

[Honza Bambas (:mayhemer)]

Attachment: v1 [merged]

- just remerged to m-c

Comment 6

[Honza Bambas (:mayhemer)]

Attachment: v1.1

- see comment 1 for overall comments

v1.1 additions:

- when a callback's onCacheEntryCheck result was WANTED_COMPLTETE we call onCacheEntryCheck again after the entry's output has been closed ; this way the callback can check again for partiality of the content and do a partial request (that is setup in onCacheEntryCheck)
- fixed the WANTED_COMPLETE test ; onCacheEntryCheck is not expected being called twice when the old backend is used

Comment 7

[Michal Novotny [:michal]]

Comment on attachment 829266 [details] [diff] [review]
v1.1

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

::: netwerk/cache2/CacheEntry.cpp
@@ +437,2 @@
>  
> +  InvokeCallbacks(true);

Do

if (InvokeCallbacks(false))
  InvokeCallbacks(true);

so that "CacheEntry::InvoikeCallback END" is always logged.

@@ +481,5 @@
> +    mCallbacks.RemoveElementAt(i);
> +
> +    if (NS_SUCCEEDED(rv) && !InvokeCallback(callback)) {
> +      // Callback didn't fire, put it back and go to another one in line
> +      mCallbacks.InsertElementAt(i, callback);

Let's say that we are processing r/w callbacks and one fails. It is inserted back and after finishing other r/w callbacks, we start invoking r/o callbacks even with the one failed r/w callback. Is this what we want to do?

::: netwerk/cache2/OldWrappers.cpp
@@ +527,5 @@
>  
>    // XXX: Start the cache service; otherwise DispatchToCacheIOThread will
>    // fail.
>    nsCOMPtr<nsICacheService> service =
>      do_GetService(NS_CACHESERVICE_CONTRACTID, &rv);

With the change above we potentially start the cache service off the main thread.

@@ +530,5 @@
>    nsCOMPtr<nsICacheService> service =
>      do_GetService(NS_CACHESERVICE_CONTRACTID, &rv);
>  
>    // Ensure the stream transport service gets initialized on the main thread
> +  if (NS_SUCCEEDED(rv) && NS_IsMainThread()) {

The same as above. We cannot guarantee where is STS initialized when the first cache load is invoked from some background thread.

@@ +587,5 @@
>        LOG(("  session->AsyncOpenCacheEntry with access=%d", cacheAccess));
>  
>        bool bypassBusy = mFlags & nsICacheStorage::OPEN_BYPASS_IF_BUSY;
> +
> +      if (cacheAccess == nsICache::ACCESS_WRITE) {

I think there should be also "&& mSync" in the condition.

::: netwerk/cache2/nsICacheStorage.idl
@@ +42,5 @@
>     */
> +  const uint32_t OPEN_BYPASS_IF_BUSY = 1 << 3;
> +
> +  /**
> +   * Do cache check (onCacheEntryCHeck) on any thread for optimal

CHeck -> Check

::: netwerk/test/unit/head_cache2.js
@@ +125,5 @@
>  
> +    if (this.behavior & COMPLETE) {
> +      LOG_C2(this, "onCacheEntryCheck DONE, return ENTRY_WANTED_COMPLETE");
> +      if (newCacheBackEndUsed()) {
> +        this.onCheckPassed = false; // we expect this to get ones more with the new cache on

It is not clear to me why the check should be called twice in case of the new cache.

Comment 8

[Honza Bambas (:mayhemer)]

(In reply to Michal Novotny (:michal) from comment #7)
> Comment on attachment 829266 [details] [diff] [review]
> v1.1

Thanks.  Some of the details discussed privately, here is some other feedback.

> ::: netwerk/cache2/OldWrappers.cpp
> @@ +527,5 @@
> >  
> >    // XXX: Start the cache service; otherwise DispatchToCacheIOThread will
> >    // fail.
> >    nsCOMPtr<nsICacheService> service =
> >      do_GetService(NS_CACHESERVICE_CONTRACTID, &rv);
> 
> With the change above we potentially start the cache service off the main
> thread.

Yes, but the only consumer that can do that is nsWyciwygChannel.  I'll add an initiation of this service there explicitly to be made on the main thread.

> 
> @@ +530,5 @@
> >    nsCOMPtr<nsICacheService> service =
> >      do_GetService(NS_CACHESERVICE_CONTRACTID, &rv);
> >  
> >    // Ensure the stream transport service gets initialized on the main thread
> > +  if (NS_SUCCEEDED(rv) && NS_IsMainThread()) {
> 
> The same as above. We cannot guarantee where is STS initialized when the
> first cache load is invoked from some background thread.

We guarantee that.  Only the http channel that is using this API fully only on the main thread also needs STS be initialized on the main thread.  So this code is OK.

Comment 9

[Honza Bambas (:mayhemer)]

Attachment: v1.1 -> 1.2 IDIFF

This is an interdiff on top of the current gum tree.  If necessary, I'll provide a full patch for review.

- taking care of initiating the old cache service and stream trans service on the main thread
- more explanatory comments added

Comment 10

[Honza Bambas (:mayhemer)]

Attachment: v2

- ENTRY_WANTED_COMPLETE renamed to RECHECK_AFTER_WRITE_FINISHED
- added more explanatory/correct comments
- special initiation of the old cache service and stream trans service on the main thread left to be added to the wyciwyg patch, since it's not needed for this patch be fully functional: wyciwyg is still using the old api and initialize the cache on the main thread (and is not using sts)
- fixed other review comments

cache2 on:  https://tbpl.mozilla.org/?tree=Gum&rev=ae6d25ad7724
cache2 off: https://tbpl.mozilla.org/?tree=Try&rev=dd92a14bd795
            https://tbpl.mozilla.org/?tree=Try&rev=7b66d8eaea88 (fixed head_cache2.js)

Comment 11

[Honza Bambas (:mayhemer)]

Attachment: v2 -> v2.1 idiff

- actually only updated the idl comments to reflect new changes at all and overall make clear how the callbacks may work

full patch is coming too.

Comment 12

[Honza Bambas (:mayhemer)]

Attachment: v2.1

Comment 13

[Honza Bambas (:mayhemer)]

Comment on attachment 831695 [details] [diff] [review]
v2.1

https://hg.mozilla.org/integration/mozilla-inbound/rev/2b59776ddb86
https://tbpl.mozilla.org/?tree=Try&rev=69d73cf7f79a

Comment 14

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/2b59776ddb86