FilePair.SetExpiry generates a 'System.ArgumentOutOfRangeException' when Expiry header contains a negative value.

[SamuelJBeer]

Bit of an edge-case issue but in the event that the Expiry header contains a negative value f.e. -1, the subsequent call to FilePair.SetExpiry() will result in an System.ArgumentOutOfRangeException being thrown, as illustrated by the following stack trace.

Exception has occurred: CLR/System.ArgumentOutOfRangeException
Exception thrown: 'System.ArgumentOutOfRangeException' in System.Private.CoreLib.dll: 'Not a valid Win32 FileTime.'
   at System.DateTime.ToFileTimeLeapSecondsAware(Int64 ticks)
   at System.DateTime.ToFileTimeUtc()
   at System.IO.FileSystem.SetLastWriteTime(String fullPath, DateTimeOffset time, Boolean asDirectory)
   at System.IO.File.SetLastWriteTimeUtc(String path, DateTime lastWriteTimeUtc)
   at FilePair.SetExpiry(Nullable`1 expiry)
   at Replicant.HttpCache.BuildResult(Timestamp timestamp, FilePair tempFile)
   at Replicant.HttpCache.<InnerAddItemAsync>d__26.MoveNext()
   at Replicant.HttpCache.<InnerAddItemAsync>d__26.MoveNext()
   at Replicant.HttpCache.<HandleFileExistsAsync>d__16.MoveNext()
   at Replicant.HttpCache.<ToFileAsync>d__49.MoveNext()
   ....

As my particular use-case was already utilising a DelegateHandler to perform header manipulation, incorporating the injection of a positive value into Expiry in the event it doesn't contain one wasn't an issue. A suggestion for a more concrete solution to this particular issue however, would be to assert that Expiry contains a positive value within the FirePair.SetExpiry method itself.

[SamuelJBeer]

On second glance, a modification to the HttpResponseMessage extension method where the header value is initially read would arguably be a better idea.

Replicant/src/Replicant/Extensions.cs

Line 163 in 3c9ce50

public static DateTimeOffset? GetExpiry(this HttpResponseMessage response, DateTimeOffset now)

[SimonCropp]

@SamuelJBeer thanks for raising the issue. do you want to have a go at submitting a pull request to fix it?

[SamuelJBeer]

Aye, I don't mind giving it a stab, @SimonCropp .

Do you have any particular preference as to the implemented strategy at all?

For instance, I'm leaning towards simply treating negative values as though they were null, however the intent behind the negative value could potentially be interpreted as indicating a resource doesn't expire and to cache it indefinitely.

Ultimately I guess it boils down to whether as a consequence of encountering a negative Expiry header value, the end result of inevitably executing FilePair.SetExpiry further down the line is that of FileEx.MinFileDate or an arbitrary point in the future.

[SimonCropp]

i would assume -1 means we should not cache it at all? so no file saved to disk?

[SamuelJBeer]

Your assumption would be invariably correct.

I had admittedly overlooked that possibility earlier as the value of Cache-Control was max-age=43200 rather than something akin to no-cache or no-store as I would've expected, but after having just picked through the header collection again, I've located the deprecated Pragma: no-cache header buried between the Heroku ones...

[SimonCropp]

can you submit a Pull Request for this