No auto delete when fd is closed on Windows

[skrysmanski]

On Windows (10), the auto-deletion of temporary files doesn't work if the file descriptor is closed manually.

The reason for this is that the "calculation" of EBADF doesn't produce the correct result on Windows. It's set to -4083 9 while it should be 9 4083. The property os.constants.errno.EBADF contains the correct value.

I'm fairly new to NodeJS so I don't feel like I could provide a proper fix (pull request) - especially considering backward compatibility with older node versions. and don't know how this should/could be solved properly.

Here's some code to reproduce the problem:

const fs = require('fs');
const os = require('os');
const _c = process.binding('constants');

const EBADF = _c.EBADF || _c.os.errno.EBADF; // Like the tmp module does it.

const CREATE_FLAGS = (_c.O_CREAT || _c.fs.O_CREAT) | (_c.O_EXCL || _c.fs.O_EXCL) | (_c.O_RDWR || _c.fs.O_RDWR);
const FILE_MODE = 384;

fs.unlinkSync('test.txt');

var fd = fs.openSync('test.txt', CREATE_FLAGS, FILE_MODE);
fs.writeSync(fd, 'test');
fs.closeSync(fd);

try {
    fs.closeSync(fd);
}
catch (e) {
    console.log('Exception: ' + e);
    console.log('ErrNo: ' + e.errno);
    console.log('Expected: ' + EBADF);
    console.log('From Constants: ' + os.constants.errno.EBADF);
}

This produces the following output:

Exception: Error: EBADF: bad file descriptor, close
ErrNo: -4083
Expected: 9
From Constants: 9

[skrysmanski]

Actually, forget my rubbish analysis above. The problem is correct but, of course, _c.os.errno.EBADF and os.constants.errno.EBADF are both 9. It's that e.errno is -4083.

[skrysmanski]

I've update the problem description. Sorry for the noise.

[silkentrance]

Which version of node do you use?

[silkentrance]

Just looked at https://msdn.microsoft.com/en-us/library/t3ayayh1.aspx

EBADF   Bad file number 9

So perhaps this is node related?

[silkentrance]

Just had a look at https://nodejs.org/api/errors.html

In there, it states that error.code contains the name of the error, in that case EBADF. Since testing against error.errno seems to be unreliable, we should be checking error.code instead. However, and since 0.11.x/0.12.x seem to behave differently, we need to make sure that we do not break backwards compatibility.

[skrysmanski]

@silkentrance I'm using NodeJS 7.5.0.

[silkentrance]

@skrysmanski since I am not working on windows and thus lack any testing capabilities, would you be so kind to test out the gh-115 branch?

Just found out that there is appveyor :sake:, however, existing test cases do not test against the user removing a file early. I will try to implement a working test case for this.

[silkentrance]

@skrysmanski the windows platform used by appveyor (2012 server) does not reproduce the error. It seems that this is very specific to Windows 10.

[skrysmanski]

@silkentrance Not sure how to use your branch using npm. So, I just manually copied the content of tmp.js into my node_modules directory.

After making some fixes (see my comment on tmp.js), the solution works on Windows 10. Thanks. :)

I'd like to make one additional suggestion: You should probably add a console.error(e) in the catch block in _garbageCollector(). (code) This would make these kind of errors more visible.

[silkentrance]

@skrysmanski regarding your input on the PR: i will have to look into this on my windows system, I just need to install the required software which may take some time.

As for the garbage collection process process during cleanup: we cannot be outputting any information to the console. Instead, we should be using an optional logger instance that can be set by the user. In case that the logger is available and supports a warn method, it will use that to report the issue.

The logger can be anything from a simple JS object, e.g.

tmp.setLogger({
  warn: function(msg) {
     console....(msg);
  }
});

to a full fledged logger instance. What do you think?

[skrysmanski]

@silkentrance

As for the garbage collection process process during cleanup: we cannot be outputting any information to the console.

Can you explain why exactly?

Instead, we should be using an optional logger instance that can be set by the user.

Actually, I (personally) would make the logger opt-out rather than opt-in (although this depends on your answer for my previous question). Because, if it's opt-in, most people will (probably) not know about this logger. And thus, any more errors like the one in this issue will still go unnoticed. But this is just my personal preference if there are no technical downsides to this solution.

[silkentrance]

@skrysmanski the here being that some users will use loggers in their web application and the information output by tmp must be written to these same logs and not just to stdout. And some users might want to write that information to stderr instead of stdout and so on.

Regardless, this is an altogether different topic and we should focus ourselves on the actual issue.

[silkentrance]

@skrysmanski sorry for taking so long but I had been busy. I have fixed the issue and also adjusted the test cases so that the issue is actually tested against. The builds on appveyor work just fine, and, with the current fix not in place, will fail as expected.

As for the review you did. The existing logic is correct in that both newer and older versions of node expose a code property, however, with different value types. newer node versions use this for storing the string, e.g. EBADF, while earlier versions of node stored the negated error code. Therefore the test against err.code == code || err.code == errno in isExpectedError(). err.errno on the other hand stores the system specific error code, or, on newer node versions, the error message. So testing against that is unreliable.

[silkentrance]

As for the logger, that requirement should have been eliminated as any failure to close an already closed file descriptor or attempt to unlink an already removed file will now be swallowed up in the remove callbacks.

Unless the user under which the current node process is running loses all permissions on the temporary files, the garbage collection process should work just fine. For the other cases we would require the external logger instance.