Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add Source.close/1 callback to prevent hitting file descriptor limits #8

Closed
wants to merge 1 commit into from

Conversation

1st8
Copy link

@1st8 1st8 commented Sep 24, 2020

Hey evadne,

thank you very much for this great package, it works fantastically and is a pleasure to work with.

I found one issue where generating large archives from local files (1000+) would crash with reason: :emfile (i.e. too many open files).
File sources were simply never closed in the Encoder loop.

My fix is to add a new Source.close/1 callback, which is called after :eof is received from the source.
This also seemed to apply to the URL and Random source, so I implemented matching behaviours there, too.

The tests were a little hard to follow for me, so this is still a work in progress and any advice would be appreciated.
I verified that the problem is fixed regarding file sources, but did not test (other than running mix test - which is green) anything regarding URL or Random.

Let me know what you think!

Cheers,
Christoph

… etc. to prevent :emfile error when creating large zips
@evadne
Copy link
Owner

evadne commented Oct 3, 2020

@1st8 what a good idea

@evadne
Copy link
Owner

evadne commented Oct 3, 2020

@1st8 IMO the way to go is to ensure the read() implementation in sources will close whatever internal handle it has

e.g.

  @impl Source
  def read(source) do
    case IO.binread(source.device, get_chunk_size()) do
      :eof -> File.close(source.device); :eof
      data -> data
    end
  end

since all streams are read once only.

evadne added a commit that referenced this pull request Oct 3, 2020
@evadne
Copy link
Owner

evadne commented Oct 3, 2020

I’ve added the logic into #8 and merged

@evadne evadne closed this Oct 3, 2020
@1st8
Copy link
Author

1st8 commented Oct 5, 2020

Cool, thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants