fs: deprecate file handle close

[rluvaton]

Warning

Work in Progress

Created after @ronag comment:

#49241 (comment)

.close should be deprecated.

---

same suggestion from @mcollina to remove the .close

• Closing fs streams early could call close during or before I/O #2006 (comment)

[ronag]

I meant on fs write stream. Also probably doc deprecate.

[rluvaton]

You meant:

• node/lib/internal/fs/streams.js

  Line 464 in 9f19253

  WriteStream.prototype.close = function(cb) {

• node/lib/internal/fs/streams.js

  Line 296 in 9f19253

  ReadStream.prototype.close = function(cb) {

instead?

[mscdex]

Why (doc or runtime) deprecate close() for node.js streams?

[bnoordhuis]

I don't get it either. A comment in the linked issue says this:

fs streams should unref FileHandle not close it

If the suggestion is to let the GC drive closing files, then that's a bad suggestion for what I hope are obvious reasons.

[benjamingr]

Why (doc or runtime) deprecate close() for node.js streams?

Why do we need to have three APIs to close a file stream .close .end and .destroy? Why should it be any different from any other stream?

If the suggestion is to let the GC drive closing files, then that's a bad suggestion for what I hope are obvious reasons.

Absolutely not. The idea from the linked issue is to enable a case where a user wants to open a steam for a FileHandle and keep the file open when the stream is closed (by explicitly opting into it!).

[benjamingr]

This PR should just be a doc-deprecation of https://nodejs.org/api/fs.html#writestreamclosecallback ?

[rluvaton]

Why only doc deprecate instead of moving node itself out of using it?

[ronag]

Throwing will cause unwarranted breakage in the ecosystem.

[rluvaton]

I did not mean to throw an error, I think that if deprecating an api we should move away from it as well

[rluvaton]

Absolutely not. The idea from the linked issue is to enable a case where a user wants to open a steam for a FileHandle and keep the file open when the stream is closed (by explicitly opting into it!).

when the user pass autoClose: false and the stream destroyed we should keep the file descriptor open?

In other words, should the following test pass?

it('should keep file handle open after stream closed when autoClose: false', async () => {
  const filePathForHandle = path.resolve(tmpDir, 'tmp-write.txt');

  const stream0Text = 'Hello world from stream0';

  const destFile = await open(filePathForHandle, 'w');
  await destFile.truncate(stream0Text.length  + 20);

  const stream0 = destFile.createWriteStream({ start: 0, autoClose: false });

  await pipeline(stream0Text, stream0);

  stream0.destroy(new Error('destroyed'));

  assert.notEqual(destFile.fd, -1);
  await destFile.close()
});

[benjamingr]

when the user pass autoClose: false and the stream destroyed we should keep the file descriptor open?

That's entirely unrelated to this PR and s just context around the linked issue

[rluvaton]

What do you think of the PR so far? Is it in the right direction?

[benjamingr]

I don't see an actual docs deprecation fo file handle close here?

[mcollina]

I think the title of this should be updated, as we are deprecating only close() for file streams, not file handles.

[ronag]

The unref isn't right. Will leave a more constructive comment next week.

[ronag]

This is wrong.