Node: 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:

https://github.com/nodejs/node/blob/dd0c5228acaf5695f96809e50771ead3167b0f44/src/node_file.cc#L146

So, the destructor calls Close():

https://github.com/nodejs/node/blob/dd0c5228acaf5695f96809e50771ead3167b0f44/src/node_file.cc#L178

which is this function:

https://github.com/nodejs/node/blob/dd0c5228acaf5695f96809e50771ead3167b0f44/src/node_file.cc#L227-L279

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.

[lundibundi]

I don't think there is anything actionable left as per the discussion above and can be closed. Feel free to reopen if anyone else has more ideas on how to improve this.