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

[WIP] fix(unlink): unlink file even the file descriptor has been closed already #47

Closed
wants to merge 5 commits into from

Conversation

raszi
Copy link
Owner

@raszi raszi commented Mar 1, 2015

Fixes #31
Fixes #52

@raszi raszi mentioned this pull request Mar 1, 2015
@raszi raszi changed the title fix(unlink): unlink file even the file descriptor has been closed already [WIP] fix(unlink): unlink file even the file descriptor has been closed already Mar 1, 2015
@raszi
Copy link
Owner Author

raszi commented Mar 1, 2015

There are other issues in the test environment which need to be addressed.

@silkentrance
Copy link
Collaborator

@raszi with all of the current patches in place, including this one, test cases will no longer leave any temporary files a/o directories, at least this was the case when I last ran npm test on node-tmp.

@silkentrance
Copy link
Collaborator

@raszi Yes, you are correct in that other error codes might be returned.

Excerpt from the man page for close():

RETURN VALUE
       close() returns zero on success.  On error, -1 is returned, and errno is set appro‐
       priately.

ERRORS
       EBADF  fd isn't a valid open file descriptor.

       EINTR  The close() call was interrupted by a signal; see signal(7).

       EIO    An I/O error occurred.

And this why we would raise the exception again in case of either EINTR or EIO or anything other that is currently not on the menu.

However, and since close() is the final operation on that file, we should try to unlink the file in the finally block and hope that this will succeed, otherwise an exception would be raised there that will then be passed to the user.

@silkentrance
Copy link
Collaborator

Looking at both #58 and #52 it seems that the original try/catch around the fs.closeSync was correct, see afe2af9#diff-4f99eaef47493ba13b06879592d2a1c4R199

It must, however, be extended to also test for ENOENT in case that the file was never created.

@silkentrance
Copy link
Collaborator

@raszi Please have a look at the mentioned issues and this pending PR.

Come on, you get a whopping 200k downloads per day on NPM 💃

@pabigot
Copy link
Contributor

pabigot commented Oct 10, 2016

I haven't tested this change, but I have found that running the test suite on master leaves files such as:

-rw------- 1 pab pab    9 Oct 10 05:24 /tmp/clike-4sx8F2-postfix
-rw-r--r-- 1 pab pab    9 Oct 10 05:24 /tmp/complicated11606JjAUMfHSrvGboptions
-rw-r----- 1 pab pab    9 Oct 10 05:24 /tmp/foo11606GlsM8RXoj4PLbar
-rw------- 1 pab pab    9 Oct 10 05:24 /tmp/something11606sMsmYnHYbleK.tmp
-rw------- 1 pab pab    9 Oct 10 05:24 /tmp/tmp-116067rh8MVJUQE7V.txt
-rw------- 1 pab pab    9 Oct 10 05:24 /tmp/tmp-11606DpO5Ey7e2bSc.tmp

/tmp/clike-esq0Fp-postfix:
total 0

/tmp/clike-p89ADH-postfix:
total 0

/tmp/complicated116065XbIidRimg8qoptions:
total 0

/tmp/foo11606rt26GugO2QiSbar:
total 0

/tmp/something116062DyohoPQzex1:
total 0

/tmp/something11606ezxoWJOdalis:
total 0

Running the suite repeated can result in bogus suite failures because the tests are unable to obtain a unique file name after the maximum number of tries.

@silkentrance
Copy link
Collaborator

Most of this has already been included in master and it no longer shows any left over files when running tests, closing.

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

Successfully merging this pull request may close these issues.

Cleanup fails npm test generates a bunch of uncleaned tmp files
3 participants