raszi / node-tmp

Temporary file and directory creator for node.js
MIT License
736 stars 92 forks source link

removeCallback() is not actually synchronous #197

Closed dieseldjango closed 4 years ago

dieseldjango commented 5 years ago

Operating System

NodeJS Version

Tmp Version

0.1.0

Expected Behavior

const tmp = require('tmp');
const fs = require('fs');

const tmpobj = tmp.fileSync();
console.log(`Created ${tmpobj.name}`);
tmpobj.removeCallback();
console.log(`Still exists: ${fs.existsSync(tmpobj.name)}`);
setImmediate(() => { console.log(`Still exists after setImmediate: ${fs.existsSync(tmpobj.name)}`); });

removeCallback() is described as synchronous. Running the code above, I would expect both console.log statements checking for the removed file to return false.

Experienced Behavior

removeCallback() isn't really synchronous, it is just an asynchronous operation that doesn't provide a promise or callback. Is this the intended behavior?

silkentrance commented 4 years ago

Internally, the operation should be synchronous. However, and as it seems, the overall behaviour of the node fs api was changed. We will need to investigate this.

silkentrance commented 4 years ago

This was fixed in #198 I believe.

silkentrance commented 4 years ago

However, I will look into the callback once again.

silkentrance commented 4 years ago

You are right. The callback returned is async. Let's see how we can fix this.

silkentrance commented 4 years ago

This might boil down to #198, still.

silkentrance commented 4 years ago

@dieseldjango you might want to try with latest master, which has the fix for #198 in place.

silkentrance commented 4 years ago

No, it was not fixed with #198:

  const removeCallback = _prepareRemoveCallback(_removeFileAsync, [fd, name], removeCallbackSync);

as it will use the async API for removing the file.

silkentrance commented 4 years ago

Perhaps we can even get rid of some code here, if we make this sync instead of async and sync on cleanup.

silkentrance commented 4 years ago

I think that with the two available interfaces, one sync and the other async, the two should return sync and async callbacks, respectively.