Use FinalizationRegistry to warn for leaked resources

[ronag]

Just playing a bit with the concept of using FinalizationRegistry to detect when/if the user has forgotten to properly release a resource and print a corresponding warning?

Something like:

const fsStreamRegistery = new FinalizationRegistry(state => {
  if (!state.destroyed) {
    process.warning('file stream leak')
  }
})

function createReadStream(...) {
  const stream = new ReadStream(...)
  fsStreamRegistery.register(stream, stream._readableState)
  return stream
}

Potential candidates:

• net.Socket
• fs.FileHandle
• fs.WriteStream
• fs.ReadStream

[addaleax]

We already implement this for fs.FileHandle, and it doesn’t apply to net.Socket because that’s a GC root.

For the fs streams, this would make sense indeed, but then again it might also be worth looking into making it use fs.FileHandle instead of raw fds?

[ronag]

making it use fs.FileHandle instead of raw fds?

This!

[ronag]

We already implement this for fs.FileHandle

Would you mind helping me find how/where this is implemented?

[ronag]

doesn’t apply to net.Socket because that’s a GC root.

Not sure I understand.

[addaleax]

We already implement this for fs.FileHandle

Would you mind helping me find how/where this is implemented?

Sure!

This line makes it so that native FileHandle objects are weak objects, which means that they are destroyed when the associated JS object is garbage collected:

node/src/node_file.cc

Line 146 in dd0c522

MakeWeak();

So, the destructor calls Close():

node/src/node_file.cc

Line 178 in dd0c522

Close(); // Close synchronously and emit warning

which is this function:

node/src/node_file.cc

Lines 227 to 279 in dd0c522

	// Close the file descriptor if it hasn't already been closed. A process
	// warning will be emitted using a SetImmediate to avoid calling back to
	// JS during GC. If closing the fd fails at this point, a fatal exception
	// will crash the process immediately.
	inline void FileHandle::Close() {
	if (closed_) return;
	uv_fs_t req;
	int ret = uv_fs_close(env()->event_loop(), &req, fd_, nullptr);
	uv_fs_req_cleanup(&req);
	struct err_detail { int ret; int fd; };
	err_detail detail { ret, fd_ };
	AfterClose();
	if (ret < 0) {
	// Do not unref this
	env()->SetImmediate([detail](Environment* env) {
	char msg[70];
	snprintf(msg, arraysize(msg),
	"Closing file descriptor %d on garbage collection failed",
	detail.fd);
	// This exception will end up being fatal for the process because
	// it is being thrown from within the SetImmediate handler and
	// there is no JS stack to bubble it to. In other words, tearing
	// down the process is the only reasonable thing we can do here.
	HandleScope handle_scope(env->isolate());
	env->ThrowUVException(detail.ret, "close", msg);
	});
	return;
	}
	// If the close was successful, we still want to emit a process warning
	// to notify that the file descriptor was gc'd. We want to be noisy about
	// this because not explicitly closing the FileHandle is a bug.
	env()->SetImmediate([detail](Environment* env) {
	ProcessEmitWarning(env,
	"Closing file descriptor %d on garbage collection",
	detail.fd);
	if (env->filehandle_close_warning()) {
	env->set_filehandle_close_warning(false);
	ProcessEmitDeprecationWarning(
	env,
	"Closing a FileHandle object on garbage collection is deprecated. "
	"Please close FileHandle objects explicitly using "
	"FileHandle.prototype.close(). In the future, an error will be "
	"thrown if a file descriptor is closed during garbage collection.",
	"DEP0137").IsNothing();
	}
	}, CallbackFlags::kUnrefed);
	}

which schedules a warning to be emitted.

doesn’t apply to net.Socket because that’s a GC root.

Not sure I understand.

Network sockets are strong objects, because they cause events to be emitted from the event loop, and therefore need to be kept alive as long as the underlying resource itself is alive. For example, if nothing on the heap refers to a socket, but there is an .on('data') listener that refers to other objects on the heap, then those objects are considered reachable because the data listener might be called and make use of these objects at any time.

[devsnek]

rooted in gc terms means the object is being referenced so it shouldn't be collected. in this case the socket is being referenced by the other end of the socket. as long as the other end is still open you probably want your side to stay open so it can get messages. this is different from an fs handle where you can completely consume the content and then forget to close it. in the native side we use persistents with finalizer callbacks to track gc.

[ronag]

Sure!

Ah it's implemented in native code. Explanation is much appreciated! Hope to jump deeper into the native parts later this year.

[devsnek]

wow @addaleax jinx

[ronag]

Network sockets are strong objects, because they cause events to be emitted from the event loop, and therefore need to be kept alive as long as the underlying resource itself is alive. For example, if nothing on the heap refers to a socket, but there is an .on('data') listener that refers to other objects on the heap, then those objects are considered reachable because the data listener might be called and make use of these objects at any time.

Hm, so if the socket is totally unreachable from user code (i.e. no alive references) it will never be GC:d, warned nor closed?

[ronag]

// leak without warning?
net.connect(opts).on('connect', socket => {
 socket.on('data', () => {})
})

[addaleax]

@ronag You could consider that a resource leak, yes, but there’s no way for Node.js to know whether the user wants that or not. The only protections we have against that are timeouts and closing the resources at process/thread exit.

[ronag]

Can't we track the JS side of the socket and warn if it becomes unreferenced? i.e. create a non root wrapper which the user interacts with?

e.g.

function connect (opts) {
  const socketRefRoot = new SocketHandle()
  const socketRef = new Socket(socketRefRoot)
  finalizer.register(socketRef, socketRef.state)
  return socketRef
}

[ronag]

but there’s no way for Node.js to know whether the user wants that or no

Ah!

[ronag]

I think my questions have been answered. I'll leave this open for a day or two in case anyone has any further input.

[addaleax]

@ronag We could provide a way to make sockets weak objects, but that would also come with the implication that those sockets might just “vanish” and stop emitting events at a random point in time, even if those events are expected and used.