bagetter / BaGetter

A lightweight NuGet and symbol server
https://www.bagetter.com
MIT License
243 stars 53 forks source link

Increase max package size to ~8GB #100

Closed Regenhardt closed 6 months ago

Regenhardt commented 7 months ago

Is there a limit we should stay in? The whole thing might be in memory while uploaded, for example in the AWS storage service, temporarily hiking RAM usage to up to ~8GB (or whatever limit we set). Should we put an info box somewhere that tells people that uploaded packages will be temporarily in memory, thus possibly spiking RAM?

Closes #101

Regenhardt commented 7 months ago

Actually the "upload" stream (which is actually a file stream to a temporary file) is copied into a MemoryStream before being passed to S3 upload, but for Filesystem storage the stream gets reset to position 0 and then just copied to the file.

Wouldn't this mean we can do the same when uploading to S3, eliminating the RAM overhead at that point?

Regenhardt commented 7 months ago

Oh and the reason the limit for each multipart-part is also the technical limit for package size is that a package, when pushed, is entirely put into one part of the multipart-message. All other parts are discarded.

seriouz commented 6 months ago

The max size of a package should be documented. 8GB NuGet is fairly huge and may be enough for the next years. The advantage of working with streams and buffers is the low RAM consumption. Even the MemoryStream has a fixed buffer size and does not eat all RAM.

Regenhardt commented 6 months ago

I'm not actually sure where in the documentation we could add this, also because 99% of people won't ever have to know this limit. Any ideas? I added a comment so people digging will find it, but I'd say let's merge it like this until we have a nice idea. New section on the homepage? New paragraph below the image like "BaGetter supports Filesystem, GCP and AWS S3 buckets for package storage, and MySQL, Sqlite, SqlServer and PostgreSQL as database. The current per-package size limit is ~8GB. It can be hosted on IIS, and is available in a linux docker container."?

Schroedingers-Cat commented 6 months ago

What do you think of making this value configurable for instance via an env variable? NuGet is used by chocolatey, so when embedding installers, those packages can become rather big.

seriouz commented 6 months ago

Yeah when using choco as package manager with BaGetter as hosting server we should not limit the package size. 8GB default but overwriteable via an optional config sounds good.

Regenhardt commented 6 months ago

Ok so there are multiple way to do this, and none of them work with the current way of configuring BaGetter it seems.

There is one class, ConfigureBaGetterOptions, that does two things:

If I just add the package size limit to BaGetterOptions and try to inject it into ConfigureBaGetterOptions to read it during web server configuration, I have a circular dependency because in order to inject it it has to be validated first, which needs to instantiate BaGetterOptions first, which now needs BaGetteOptions...

  1. I could instead inject IConfiguration into ConfigureBaGetterOptions and read the value right there. This feels like breaking abstraction layers.
  2. I could create another options class, configure that, not add it to the validation class, and inject that. This feels like un-centralising the options and leaking validation. The whole thing is now one dimension more complex.
  3. Split up ConfigureBaGetterOptions into ConfigureBaGetterServer and ValidateBaGetterOptions, then inject BaGetterOptions into ConfigureBaGetterServer. This also un-centralises, but I feel like configuration of the web server and validation of the options are far enough from each other that splitting them is ok.

You have been nice rubber duckies, I'll go with option 3 for now, thank you. I will still take comments about other options or different names for ConfigureBaGetterServer and ValidateBaGetterOptions though 😃 .

seriouz commented 6 months ago

Sorry but I like option 3 the most too. :D

Regenhardt commented 6 months ago

I added it to the docs too, hope it fits there.