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

need to close Reader after parsing is finished #13

Closed
ntuckerxx opened this issue Apr 18, 2014 · 3 comments · Fixed by #42
Closed

need to close Reader after parsing is finished #13

ntuckerxx opened this issue Apr 18, 2014 · 3 comments · Fixed by #42

Comments

@ntuckerxx
Copy link

id3() creates a new Reader and calls its .open method but never closes it. If you don't cycle through lots of files quickly, this does not seem to be a problem (presumably because the Reader eventually gets destroyed and the file handle along with it), but if you churn through a lot of files very quickly, this can cause you to run out of file handles and id3 eventually starts failing with "Could not open specified file." The fix is simply to put a call to handle.close() inside the ID3Tag.parse callback.

@43081j
Copy link
Owner

43081j commented May 11, 2014

There is a handle.close() in the ID3Tag.parse callback, here.

Did you mean elsewhere?

@ntuckerxx
Copy link
Author

Ah, yes, but that version of the code is not what you get when you npm install id3js. So the bug still exists in the npm-published version. I guess this bug should be "publish 1.1.3 to npm please." :)

BTW, is there some reason you've excluded your 'build' directory from the repo? I've forked to fix another issue and I've been just editing the big concatenated 'dist/id3.js' file directly, but I assume that's not how you do it. I meant to contact you about this before sending a pull request, but I hadn't gotten around to it.

@43081j
Copy link
Owner

43081j commented May 11, 2014

Sorry about that, its a bit of a mess.

Being that it began as a much smaller lib, I initially didn't have a solid build process. Then parts of it got forked off to other repos (dataview, reader.js) and now its a bit of a mess and a pain to update. Really, they should be deps now but we can't just npm them as this works for browsers too.

You can edit dist/id3.js but just remember some of the code is from dependencies, so if you feel the need to change those, there's likely a bug to be raised in their repos rather than this.

@43081j 43081j mentioned this issue Jun 17, 2019
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 a pull request may close this issue.

2 participants