yesodweb / wai

Haskell Web Application Interface
MIT License
832 stars 261 forks source link

Last-Modifed and If-Modified-Since handling with `responseFile` #978

Open maxime-didier opened 7 months ago

maxime-didier commented 7 months ago

The problem

We are serving static files from a nix store path using wai-app-static, which relies on responseFile by default to send the contents over. The way this works, deployments are immutable folders in which every file has a timestamp of 1 (1970-01-01T00:00:01Z), an update is made by either updating a symlink somewhere or updating which nix store path is being served.

We have some custom logic to derive ETags from nix store paths, and wai-app-static handles If-None-Match headers accordingly. But this does not work because warp's handling of a responseFile without a specified FilePart relies on filesystem modification dates (Last-Updated + If-Modified-Since headers) and mistakenly overrides wai-app-static's cache handling.

The end result is that warp incorrectly sends 304 responses even though the file has actually changed and has a different ETag associated to it. This results in stale content being used from the browser's cache.

We have looked at warp's configuration options and have not found a way to disable this behavior.

Our workaround

At first, we instructed our internal users to issue a Ctrl+F5 to force their browsers to refresh their content cache. We've since setup our reverse proxy to strip the Last-Updated and If-Modified-Since headers from responses and requests respectively for the impacted services. This works pretty well and hasn't caused other issues so far.

Long term solution

I can see a few ways this issue could be solved. One would be to have warp check if ETags headers are present and avoid relying on modification times in such cases. Another would be to add a configuration option to make it possible to disable and/or customize how warp deals with filesystem modification times. I do not have strong beliefs on how this issue should be handled, so suggestions are welcome.

Once an approach has been decided, we are willing to submit a PR implementing it as soon as we are able to.

Vlix commented 5 months ago

I've tried to find which parts of the code you're having trouble with, but I'm still not sure what the issue is.

Could you mention files/lines (maybe copy paste sections of code) to more specifically point to the parts that are the cause and what the issues are per section?

maxime-didier commented 5 months ago

The relevant wai-app-static line is here. There is a relevant comment in the warp implementation here, and the code is there.

Vlix commented 2 days ago

@maxime-didier Do I understand correctly that the issue is that warp ALWAYS adds a Last-Modified header when sending a "file without a FilePart"? Because if the requester doesn't send an If-Modified-Since, it should work as expected. And since you're using ETags, I'd expect the requester to use If-Match or If-None-Match.

But I guess the requester is not under your control? (Like a browser?) And if warp sends a Last-Modified, then it will use If-Modified-Since?

maxime-didier commented 1 day ago

This is indeed part of the issue. Our custom logic adds an ETag header, and warp adds a Last-Modified. The client receives both of these headers and this puts us in this unfortunate situation.

There's another moving part to this issue, though. A browser that received both headers will implement caching by sending both an If-None-Match and an If-Modified-Since and will continue to do so until the cache entry expires or it receives an updated resource. This means that removing the Last-Modified header from initial responses will not be enough to fix the problem if warp still preempts custom caching logic if it sees an If-Modified-Since. This is not a trivial issue because IIRC, the default cache expiration time is set to a far date into the future.

The core problem is that warp assumes it can rely on file modification times to implement caching. The Nix store is one example where this assumption fails, there may be others, ie. a filesystem that does not implement modification times.

Vlix commented 18 hours ago

Ok, I'm trying to find the spot where the override happens.

If both the If-None-Match and If-Modified-Since are given, it should produce a regular OK 200 (at least in the current code)

EDIT: This has been the case since warp-3.3.30