WritableStream leaks a descriptor if it is not ended

[mmomtchev]

Version

12-14-16

Platform

All

Subsystem

Streams

What steps will reproduce the bug?

Consider this code:

const axios = require('axios');
const fs = require('fs');
const stream = require('stream');
const { promisify } = require('util');
const streamFinished = promisify(stream.finished);

const dl = axios.create({
	responseType: 'stream'
});

(async () => {
	do {
		console.log('iteration');
		try {
			const writer = fs.createWriteStream('temp');
			const reader = await dl.get('https://www.google.com/give_me_404');
			reader.data.pipe(writer);
			await streamFinished(writer);
		} catch (e) {
			console.log(e.message);
		}
	} while (true);
})();

It will leak a descriptor at every iteration. This has nothing to do with axios - in fact this is enough to leak:

const fs = require('fs');

do {
	console.log('iteration');
	const writer = fs.createWriteStream('temp');
} while (true);

How often does it reproduce? Is there a required condition?

Always

What is the expected behavior?

Destruction of WritableStream that is not piped (by going out of scope) should free the file descriptor

What do you see instead?

The problem is that in the first example the only way to not leak a descriptor is:

const axios = require('axios');
const fs = require('fs');
const stream = require('stream');
const { promisify } = require('util');
const streamFinished = promisify(stream.finished);

const dl = axios.create({
	responseType: 'stream'
});

(async () => {
	do {
		console.log('iteration');
                let writer, reader;
		try {
			writer = fs.createWriteStream('temp');
			reader = await dl.get('https://www.google.com/give_me_404');
			reader.data.pipe(writer);
			await streamFinished(writer);
		} catch (e) {
                        try {
                             reader.close();
                        } catch () {}
                        try {
                             writer.close();
                        } catch () {}
			console.log(e.message);
		}
	} while (true);
})();

This is the only way to never leak a descriptor and to handle all type of errors that can happen.

Additional information

No response

[lpinca]

It will leak a descriptor at every iteration. This has nothing to do with axios - in fact this is enough to leak:

const fs = require('fs');

do {
	console.log('iteration');
	const writer = fs.createWriteStream('temp');
} while (true);

To be honest I think this is expected. The stream has not finished nor errored.

[mmomtchev]

@lpinca It render the Streams API very quirky to use - it is what was leaking in my case, but it is probably not the only case - and this coupled with Node's exceptional ability to conceal FD leaks, renders even huge leaks very easy to miss.

I guess that there are lots of user applications out in the wild that horribly leak descriptors - mostly on their error code paths.

AFAIK, the current Streams API makes it possible to avoid leaks - I went through a lot of my code the last few days - and I never found one case where it was not possible to plug the leak - but the solution is often not intuitive and never documented.

[jasnell]

Yeah, this is a known (and unfortunately expected) limitation of fs.WriteStream and fs.ReadStream. If the objects are allowed to be garbage collected before they are closed, their file descriptors will never be closed. This, btw, is a large reason why FileHandle exists and why the fs.promises API uses it instead of raw file descriptors. @addaleax implemented some logic a while back for Worker Threads to automatically track unmanaged fd's and close those when the worker is closed. It's conceivable that we could do something similar here also. Specifically, using WeakRefs and FinalizationRegistry we could track fd's associated with fs.WriteStream and fs.ReadStream objects and either automatically clean those up or complain loudly the way FileHandler does.

[addaleax]

This, btw, is a large reason why FileHandle exists

Yeah, and imo, that’s part of the solution here – instead of using plain fds, we could/should implement the streams in terms of FileHandle as the underlying resource. We actually have pretty much all code for that in place already, it would not be hard at all to implement for the common create{Write,Read}Stream(filepath) case.

Specifically, using WeakRefs and FinalizationRegistry we could track fd's associated with fs.WriteStream and fs.ReadStream objects and either automatically clean those up or complain loudly the way FileHandler does.

The main reason that we’ve never done this is that GC is not a reliable way to manage resources other than memory, and unfortunately, that hasn’t really changed.

[ronag]

const { pipeline } = require('stream/promises');
const reader = await dl.get('https://www.google.com/give_me_404');
const writer = fs.createWriteStream('temp');
await pipeline(reader.data, writer);

[ronag]

IMO, relying on GC is bad practice.

[mmomtchev]

What is the problem with relying on the GC? Are there any cases where destroying the Stream object should not also close the underlying FD when it is not already closed? As far as the case where the GC will simply skip some objects because it is in a hurry - this is not a huge problem - these are not lost, they are still tracked, and eventually, at some point, should they become far too many, they will have to be destroyed.

[ronag]

should they become far too many, they will have to be destroyed.

File descriptors don't put pressure on GC. So in theory you could allocate millions of them without GC ever running. You should never rely on GC as it is undeterministic.

[ronag]

We have the same problem in e.g. fetch https://github.com/nodejs/undici#garbage-collection

[jasnell]

Oh, certainly, without a doubt relying on GC to clean things up is bad. However, using it similarly to what we do in FileHandle... that is, complain loudly when something wasn't closed correctly, is at least viable. It doesn't help with the actual management, however.