nix icon indicating copy to clipboard operation
nix copied to clipboard

S3 downloads are not interruptable

Open edolstra opened this issue 1 year ago • 2 comments

Describe the bug

For instance, hitting Ctrl-C during

nix eval --impure --expr 'builtins.fetchurl "s3://highlighter-public/405b3ba02ccb2f07ec6aba9f233d4194.mp4?region=ap-southeast-2"'

does nothing.

Steps To Reproduce

Expected behavior

Nix should exit with error: interrupted by the user quickly.

Probably the easiest way to do this is to have S3Helper::getObject() use an std::stringstream wrapper that calls checkInterrupt().

nix-env --version output

Additional context

Add any other context about the problem here.

Priorities

Add :+1: to issues you find important.

edolstra avatar Oct 11 '24 12:10 edolstra

@edolstra ill work on this issue if you can assign me this

zacharyftw avatar Oct 11 '24 17:10 zacharyftw

As a first-time contributor, I'd like some help. Where should I implement the new InterruptibleStringStream class (like in libutils or a specific folder)? Also, is this 👇🏽 where is the S3Helper is implemented ? https://github.com/NixOS/nix/blob/dbcd4cd6bab50ffbad795feec404758644415815/src/libstore/s3-binary-cache-store.cc#L92

zacharyftw avatar Oct 11 '24 18:10 zacharyftw

I'm somewhat confused by the current implementation. Why does S3 fetching live inside curlFileTransfer, while having absolutely nothing to do with curl. The code is already littered with FIXME/TODO and the "easy" fix would pile even more complexity on this class. SRP is totally broken here. Maybe the better thing to do would be to define an abstract interface for fetching and move the code related to S3 into its own implementation?

xokdvium avatar Nov 08 '24 11:11 xokdvium

Also S3 fetching is already problematic and it's not clear how it handles compressed streams? Does the AWS SDK handle the decompression (can it even do that?) or should it be handled the same way that it's done for regular HTTP with libarchive and TarArchive and Sinks/Sources? https://github.com/NixOS/nix/issues/11261

It would be very helpful to define interface boundaries so that there are clear and defined requirements and api for file transfers/stream decompression/progress tracking. This would be better in the long run than piling amazingly complex logic into a single class.

xokdvium avatar Nov 08 '24 11:11 xokdvium

I've made an attempt in #12227, it's a little delayed at closing out but it does seem to improve the problem.

RossComputerGuy avatar Jan 10 '25 22:01 RossComputerGuy

Fixed by #13752

lovesegfault avatar Oct 17 '25 16:10 lovesegfault