[core] Style request doesn't get retried on mbgl::NetworkStatus::Reachable()

[tmpsantos]

Steps to trigger behavior

1. Set a style URL with the network connection down and empty cache.
2. Turn the network connection up.
3. Call mbgl::NetworkStatus::Reachable()

Expected behavior

Style request gets retried and we see a map.

Actual behavior

Blank map.

[jfirebaugh]

Should this condition be expires ? *expires > util::now() : !error? In case there is an explicit Cache-Control or Expires header on an error response, it should be respected.

Also, error can be Reason::NotFound (404). We should carefully consider (and test) what the retry behavior is in that situation.

[tmpsantos]

Should this condition be expires ? *expires > util::now() : !error? In case there is an explicit Cache-Control or Expires header on an error response, it should be respected.

I think they are logically the same. But yours is definitely easier to read.

Also, error can be Reason::NotFound (404). We should carefully consider (and test) what the retry behavior is in that situation.

Currently NotFound doesn't trigger a retry which makes sense, specially for tiles. I can't think of a use case we should retry for styles.

I added 2 more tests to make sure this is what we want.

[jfirebaugh]

I think they are logically the same. But yours is definitely easier to read.

In the case that error and expires are both set, (!expires || *expires > util::now()) && !error is always false, but expires ? *expires > util::now() : !error compares expires to the current time and ignores error. I'm arguing that the latter is the desired behavior.

Currently NotFound doesn't trigger a retry which makes sense, specially for tiles.

I don't think this is the case. Imagine the scenario where someone uploads a tileset for a small area, and then expands it to cover a larger area. If you cached a 404 for tiles outside the original boundary, you'd want to be able to obtain the newly available tiles when that cached 404 expires.

[tmpsantos]

In the case that error and expires are both set, (!expires || *expires > util::now()) && !error is always false, but expires ? *expires > util::now() : !error compares expires to the current time and ignores error. I'm arguing that the latter is the desired behavior.

Now I see. expires ? *expires > util::now() : !error is indeed the desired.