Out of file descriptors!

[hsyl20]

When packing too many "big" files, packFileEntry' may lead to FD starvation. That's because it uses lazy I/O.

This happened in cabal-install when packing haddock generated documentation: see https://gitlab.haskell.org/ghc/ghc/-/issues/25172#note_630488

How to reproduce

Try to pack N+1 "big" files with ulimit -n N

[Bodigrim]

I'd hope that copious amounts of unsafeInterleaveIO will keep it from opening more than one handle at once, but apparently it's not the case. I'll take a look later this week.

[Bodigrim]

It seems that commenting out traverse_ (maybe (pure ()) throwIO . secCB) entries helps (presumably because this is a line which forces all IO actions and is not covered by unsafeInterleaveIO)

[Bodigrim]

An "obvious" solution would be to interleave maybe (pure ()) throwIO . secCB as in 5576533, but it's likely to defeat the purpose: given that the computation returns (), such unsafeInterleaveIO can be skipped entirely.

The only reasonable action seems to be parametrizing GenEntryContent by "content", which can be either a lazy ByteString or a filename. This is a breaking change, of course.

[mpickering]

Another option would be to add a parameter to "pack" which allows the library author to control at what size the files are read strictly.

@Bodigrim Is this commit (990b3ea) relevant to this issue? It seems to change the definition of packPaths.

[Bodigrim]

@mpickering it is "related" as in "I made it while pondering on this issue". I think though that 990b3ea does not change the behaviour, am I wrong?

I'm sorry, I cannot find a big enough chunk of time to dedicate to this issue properly.

One thing I tried and failed was to gzip strictly-read data on the fly. In theory it would allow us to raise the threshold size without increasing peak memory residency. Yet somehow memory requirements only grow up, I don't understand why.

[mpickering]

You are right that 990b3ea doesn't seem to change the behaviour, I missed that interleave was calling itself so that the unsafeInterleaveIO were in the same place.

It's ok, not expecting you to fix the issue but if a plan is established between us then someone else will be able to easily fix it. Coming up with the plan is probably the hardest bit.

[Bodigrim]

Another option would be to add a parameter to "pack" which allows the library author to control at what size the files are read strictly.

I'm reluctant to go this route: it's good enough as a temporary hack, but it will get enshrined in Cabal almost immediately, making it harder to fix properly.

It's ok, not expecting you to fix the issue but if a plan is established between us then someone else will be able to easily fix it. Coming up with the plan is probably the hardest bit.

Fundamentally we'd like to eradicate any intermediate LazyByteString. We need to keep a LazyByteString-based legacy interface for compatibility, but there should be a way to create an entire archive without even touching lazy I/O.

I imagine a way forward would be to parametrize

data GenEntryContent content linkTarget
  = NormalFile      content !FileSize
  | Directory
  | SymbolicLink    !linkTarget
  | HardLink        !linkTarget
  | CharacterDevice !DevMajor !DevMinor
  | BlockDevice     !DevMajor !DevMinor
  | NamedPipe
  | OtherEntryType  !TypeCode content !FileSize

so that one can set content ~ FilePath and read files later, directly before writing archive.

But what about FileSize fields? Currently we do not trust filesystem to report up-to-date sizes: we rely on hFileSize hndl; and obviously file size can change between creating an index of GenEntry and actually writing them. Adding yet another type parameter filesize ~ FileSize / filesize ~ () feels too convoluted. I'm currently leaning to trust the filesystem and populate NormalFile filepath <$> getFileSize filepath; if things shifted under our feet and file size has changed once we actually read the file, we can throw an error.

@mpickering what do you think?

[Bodigrim]

Fixed in #102.