Fix for #498 - added OutputCache attribute

[matthewskelton]

As discussed over in #498, I've added the following:

[OutputCache(Duration = Int32.MaxValue, Location = OutputCacheLocation.Downstream)]

We can cache the results 'permanently', but only at server/proxy intermediate layers (not at the client).

[akoeplinger]

Correct me if I'm wrong, but I think this fix will only apply to local installations of the NuGetGallery, because the main nuget.org site uses Windows Azure and the GetPackage action just redirects to the Blob storage. As mentioned in a StackOverflow discussion, caching doesn't work for RedirectResult.

Nonetheless, for local installations it'll be perfectly OK to cache the response indefinitely as it's not possible to upload an updated package with the same version, like you said.

[matthewskelton]

@akoeplinger Thanks for the info. I'm definitely targeting this at local installations of NuGet Gallery rather than the nuget.org instance, although it still could be relevant there (if the storage ever changes).

[akoeplinger]

@matthewskelton Good point. I'll plan on using NuGet Gallery for one of my projects soon, so this will definitely improve the performance.
As a side note, you mentioned in #498:

In terms of OutputCacheLocation, I think we should avoid telling clients that they can cache it, as we cannot predict how NuGet client needs will evolve over time, hence Downstream for this.

According to the MSDN, a value of Downstream however means

The output cache can be stored in any HTTP 1.1 cache-capable devices other than the origin server. This includes proxy servers and the client that made the request.

I think it's perfectly valid for the client to also cache the nupkg forever, because even if the format changes etc, the existing packages won't be updated anyway. Can you clarify why you think clients should not cache the package?

[matthewskelton]

@akoeplinger Hmm, well spotted; I missed the crucial "and the client" on that page.

However, Downstream is still the most appropriate caching strategy, imo: given that both Any and Downstream set the Cache-Control header to public, the only difference at the ASP.NET level must be that with Downstream, the origin server does not cache the result. This is a good thing, since as soon as the proxy server has cached the response, the origin server will never see a request for the package again, so caching it is just a waste of origin server resources.

I don't think that the client should never cache the content, just that I'd have liked to restrict the cacheability changes I am pushing to avoid side-effects at the client-side. However, (having refreshed my memory on this!) HTTP 1.1 does not support "cache everywhere except the client", and well-implemented clients should handle the Cache-Control: public correctly, so I don't think this is a big issue.

[akoeplinger]

@matthewskelton You're right, caching on the server will be a waste of resources in this case as the proxy/client should cache it indefinitely.

I just noticed another point that wasn't considered yet: download statistics. The GetPackage controller action adds an entry into the PackageStatistics table, which is later on used for computing the number of downloads for a package. If the proxy caches the result, only one request will ever reach the server, so the download counter stays at 1.

[matthewskelton]

@akoeplinger Interesting. I think this shows how - so far - the assumed deployment model for NuGet Gallery is that the ASP.NET application itself will front all the requests. I raised #499 to cover another scenario relating to Gallery sitting behind a caching proxy, so there is clearly some interesting work to be done in this area :)

Do you want to raise a ticket to capture the PackageStatistics issue, or shall I?

[akoeplinger]

I just filed an issue here: #506

[matthewskelton]

@akoeplinger Thanks - looks good.

[half-ogre]

You are correct; we currently haven't planned to move package downloads to a CDN or other cache. It's something we've talked about, but nothing more. The question, then, is whether we want to add downstream caching until we've better considered this. Moving to CDN gives a way to pull in the stats from the CDN and add it to our stats. But with downstream caching, the tick will never get sent. As you mention in #506, that might well be okay, and perhaps we need to consider changing how we track stats. Until the team has considered it though, I think we need to hold off on this change. I'll keep the PR up until we do. Thanks!

[akoeplinger]

I agree that downstream caching should only be enabled when the statistics problem is solved.
I'd vote for the addition of an "installation count" metric, but as this is no small change and needs adjustments in both the NuGet client and server, it might take a while to implement. I'm happy with leaving this PR open until then 😄

[haacked]

Why don't we close the PR for now and re-open when we're ready to take these changes? Keeping it open just pollutes the queue.

I'll do that and if you disagree, feel free to comment and re-open. :)