1146585 - Add a test for Cache.delete

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Comment 1

[(no longer active)]

Attachment: Add a test for Cache.delete

Comment 2

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8581903 [details] [diff] [review]
Add a test for Cache.delete

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

r=me with the race addressed.  Thanks!

::: dom/cache/test/mochitest/test_cache_delete.js
@@ +78,5 @@
> +  var cache;
> +  return setupTest(tests)
> +    .then(function(c) {
> +      cache = c;
> +      // Simultaneously add an delete a request

nit: add and delete

@@ +81,5 @@
> +      cache = c;
> +      // Simultaneously add an delete a request
> +      return Promise.all([
> +        cache.add(newURL),
> +        cache.delete(newURL),

I'm not sure the behavior of this is strictly defined.  The `cache.add()` is started first.  It could in theory complete before the `cache.delete()` runs.  I think you are testing a race here.

Now, if you put the delete() first in the Promise.all(), then you are guaranteed that it should always fail.

Comment 3

[(no longer active)]

(In reply to Ben Kelly [:bkelly] from comment #2)
> @@ +81,5 @@
> > +      cache = c;
> > +      // Simultaneously add an delete a request
> > +      return Promise.all([
> > +        cache.add(newURL),
> > +        cache.delete(newURL),
> 
> I'm not sure the behavior of this is strictly defined.  The `cache.add()` is
> started first.  It could in theory complete before the `cache.delete()`
> runs.  I think you are testing a race here.
> 
> Now, if you put the delete() first in the Promise.all(), then you are
> guaranteed that it should always fail.

Hmm, yeah my reading of the spec was incorrect here.  But I don't understand why reversing the order of add and delete would make the race go away.  This is basically a race between fetch and the parallel steps of Batch Cache Operation, right?  What am I missing?

Comment 4

[Ben Kelly [:bkelly, not reviewing]]

(In reply to :Ehsan Akhgari (not reading bugmail, needinfo? me!) from comment #3)
> Hmm, yeah my reading of the spec was incorrect here.  But I don't understand
> why reversing the order of add and delete would make the race go away.  This
> is basically a race between fetch and the parallel steps of Batch Cache
> Operation, right?  What am I missing?

I believe it would make them go away because delete() would enter Batch Cache Operation before add() would enter it.  When dealing with just meta data (not body data), those steps are essentially synchronous.  So it implies to me which ever starts adjusting meta data first wins.

And currently its not just a race with the fetch part of add.  If you fetch'd first and used put, the race would still exist because we don't write the put() meta data till the body is fully loaded.  This particular race, though, will go away once we implement committing the put when headers are available.

Comment 5

[(no longer active)]

OK, I see.  Thanks!

Comment 6

[(no longer active)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/7386c130fb78

Comment 7

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/7386c130fb78