994606 - HTTP cache v2: have test to check persistent is correct

Status: NEW

(Core :: Networking: Cache, defect, P5)

Description

[Honza Bambas (:mayhemer)]

Bug 994574 shows we need to check this.  We actually have APIs to do the actual tests.  An xpcshell test that has a profile, stores entries + data, then purges all from the memory pool (we have public APIs!) and then reopen the entry.  Expected is to find the entry.  Simple.

Tentatively blocking for enabling, since this is actually critical.

Comment 1

[Honza Bambas (:mayhemer)]

Hmm.. but 1) purging is async 2) entries will be referenced that time, so it won't work anyway..  Other idea is to let the cache store stuff somewhere else so that it can be found by a following test.  Or, have some testing only API that will just restart the cache (purge synchronously, close everything forcefully and give a callbacks its done)

Comment 2

[Honza Bambas (:mayhemer)]

Lack of time, test manually now.

Comment 3

[Michal Novotny [:michal]]

Attachment: test

https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=0b773da9ee92

I chose a simple solution when we copy a pre-generated entry file to the cache directory before the service is started. I also decided to include a security info in the entry and test its deserialization, because it happened already twice (bugs 643041 and 764496) in the last few months that the format of the serialized security info has changed and it took us some time to investigate users' bugreports (bugs 1083922 and 1116295).

Comment 4

[Michal Novotny [:michal]]

Attachment: script for generating the entry file

Is there an easier way to get a valid security info? The code (taken from test_tls_server.js) starts tls server, makes the connection to it and takes the security info from client's transport.

Comment 5

[Honza Bambas (:mayhemer)]

Comment on attachment 8545975 [details] [diff] [review]
test

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

This only tests we read from the disk correctly.  What is your plan to check we also persist correctly?  I think storing an entry and then after flush/purge (have a test "flush" api) do a new load would do.

Comment 6

[Honza Bambas (:mayhemer)]

Comment on attachment 8545979 [details]
script for generating the entry file

dropping f? until question from the previous comment is answered.

Comment 7

[Michal Novotny [:michal]]

(In reply to Honza Bambas (:mayhemer) from comment #5)
> This only tests we read from the disk correctly.  What is your plan to check
> we also persist correctly?  I think storing an entry and then after
> flush/purge (have a test "flush" api) do a new load would do.

My plan was to check only parsing of the entry. I think that doing this flush is not a simple task. Actually, it should do a clean shutdown (on all levels) followed by a startup, but the clean shutdown is a tricky part. The current code does not guarantee that all cached data is flushed to the disk before we shutdown the cache IO manager. _Normally_ all references to CacheEntry, CacheFile, CacheFileChunk, etc. are released (and the data written) before CacheFileIOManager::ShutdownInternal() is executed, but in case some reference is not released the data might not be written to disk, the handle is forcibly closed and the incomplete file removed.

If we were able to set LastFetched and LastModified attributes of the cache entry, we would be able to test the writing of the entry in a similar way how this patch tests reading of the entry from the disk. I.e. we would create an exact entry that we would compare with an existing file.

Comment 8

[Honza Bambas (:mayhemer)]

(In reply to Michal Novotny (:michal) from comment #7)
> (In reply to Honza Bambas (:mayhemer) from comment #5)
> > This only tests we read from the disk correctly.  What is your plan to check
> > we also persist correctly?  I think storing an entry and then after
> > flush/purge (have a test "flush" api) do a new load would do.
> 
> My plan was to check only parsing of the entry. I think that doing this
> flush is not a simple task. Actually, it should do a clean shutdown (on all
> levels) followed by a startup, but the clean shutdown is a tricky part. The
> current code does not guarantee that all cached data is flushed to the disk
> before we shutdown the cache IO manager. _Normally_ all references to
> CacheEntry, CacheFile, CacheFileChunk, etc. are released (and the data
> written) before CacheFileIOManager::ShutdownInternal() is executed, but in
> case some reference is not released the data might not be written to disk,
> the handle is forcibly closed and the incomplete file removed.

Would we be alright with just a notification that "all has been flushed"?  We could use the existing Purge(ALL) API on the service to push out everything from memory, as it is now, no need to change any implementation.  Only extension would be an optional callback to Purge() that after everything is actually out (no handles - is that enough?) gets notified we are "flushed".  Then the test may continue.  

(The Purge() call could somehow push the metadata/data? write timers forward, followup sufficient.)

> 
> If we were able to set LastFetched and LastModified attributes of the cache
> entry, we would be able to test the writing of the entry in a similar way
> how this patch tests reading of the entry from the disk. I.e. we would
> create an exact entry that we would compare with an existing file.

I see.  Sounds a bit like a hack tho - what if we break exactly one of those two?

Comment 9

[Honza Bambas (:mayhemer)]

Note: these days we have the "cacheservice:purge-memory-pools" observer service notification.  When you purge everything, after this notification you can check the disk content loads back as expected.  Only thing to do is to correctly garbage collect all the entries, doable tho.

Comment 10

[Firefox Bug Husbandry Bot]

Bulk change to priority: https://bugzilla.mozilla.org/show_bug.cgi?id=1399258

Comment 11

[Valentin Gosu [:valentin] (he/him)]

Unassigning Michal to move bugs back to triage pool.