Concurrent requests for the same resource can result in cache misses

[asajeffrey]

If two GET requests are processed in quick succession, then the second one can end up being a cache miss, since the code to check whether the cache has been hit uses a read lock at

servo/components/net/http_loader.rs

Line 1011 in 6ed204d

if let Ok(http_cache) = context.state.http_cache.read() {

whereas the code to insert into the cache uses a write lock at

servo/components/net/http_loader.rs

Line 1120 in 6ed204d

if let Ok(mut http_cache) = context.state.http_cache.write() {

[asajeffrey]

IRC discussion here https://mozilla.logbot.info/servo/20190909#c16601392

[asajeffrey]

This came up in the context of #24148 which makes concurrent GETs more likely.

[asajeffrey]

The matching spec issue: whatwg/fetch#932

[gterzian]

That's interesting.

it might be the point that our simple "lock around a hashmap" approach to the cache shows its limitations.

We could handle this quite easily if the cache was message-passing based, where the cache ran in a worker thread, and each fetch thread would send a message to ask for a cached entry, and block on the reply. The cache could then simply let the fetch block if it knows there is another one doing the fetch at the same time.

I guess it could be done with locks(and condvars?) without an additional cache worker thread, but that might be much more complicated...

[jdm]

I don't think a message-passing-based approach would solve this issue - the spec describes two separate operations (check the cache & store the response) which happen at different times, so unless we add a marker entry to the cache which means "something else has requested this URL and there is no response yet" we're going to always run into this problem.

[gterzian]

so unless we add a marker entry to the cache which means "something else has requested this URL and there is no response yet" we're going to always run into this problem

I agree. What I mean is that it would be easier to add such a marker, as it would any other "state" in addition to the cached entries, if the cache where to run in it's own thread and handle messages, versus adding more shared-state to the current structure.

[gterzian]

Ok so I think one idea from https://github.com/servo/servo/pull/23553/files could be useful, which is to have fetches operate on entries, versus the cache as a whole.

Because a problem with the current structure is that we're operation on a RwLock<HttpCache>, which means it's going to be pretty hard to hide any sort of condvar based logic inside of it, since you'd need to acquire the outer lock first, and to tie that one to a condvar would require changing the whole structure of HttpState.

Instead, if fetches operate on entries, we can put a condvar inside of it, tie it to a new mutex around a "pending fetch" flag, and just have every other fetch using the same entry block on it, without affecting ALL fetches.

(And we don't need all the fine-grained locking from that PR, just the entries concept).

[gterzian]

I will take a look at using entries, and a condvar.