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

removeCallback() is not actually synchronous #197

Closed
1 of 8 tasks
dieseldjango opened this issue Jun 29, 2019 · 9 comments · May be fixed by mike-north/devcert#45 or sekikn/avro#9
Closed
1 of 8 tasks

removeCallback() is not actually synchronous #197

dieseldjango opened this issue Jun 29, 2019 · 9 comments · May be fixed by mike-north/devcert#45 or sekikn/avro#9
Labels

Comments

@dieseldjango
Copy link

dieseldjango commented Jun 29, 2019

Operating System

  • Linux
  • Windows 7
  • Windows 10
  • MacOS
  • other:

NodeJS Version

  • 0.x
  • 4.x
  • 6.x
  • 7.x
  • other: 10.16.0

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
Copy link
Collaborator

silkentrance commented Sep 20, 2019

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
Copy link
Collaborator

This was fixed in #198 I believe.

@silkentrance
Copy link
Collaborator

However, I will look into the callback once again.

@silkentrance
Copy link
Collaborator

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

@silkentrance
Copy link
Collaborator

This might boil down to #198, still.

@silkentrance
Copy link
Collaborator

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

@silkentrance
Copy link
Collaborator

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
Copy link
Collaborator

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

@silkentrance
Copy link
Collaborator

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

silkentrance added a commit that referenced this issue Jan 27, 2020
silkentrance added a commit that referenced this issue Jan 27, 2020
fix #197: return sync callback when using the sync interface, otherwise return the async callback
silkentrance added a commit that referenced this issue Jan 29, 2020
…c or rmdirSync during garbage collection and not use a next parameter
silkentrance added a commit that referenced this issue Jan 29, 2020
fix #230: regression after fix for #197
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
2 participants