Not cleaning things up...

[jimmywarting]

This lib do not seem to cleanup stuff on long running processes that never exits (like a server), it only clean up on exit...?
either that or i'm doing something wrong...

i created a simple fiddle. but it never executes the shutdown fn
https://stackblitz.com/edit/node-hnuwfg?file=index.js

[mcollina]

Ahum I did some tests, and I do not see any memory leak. The "big" array you are allocating is collected, therefore the shutdown function is never called (because that object is not there anymore). Here is my code:

const { register, unregister } = require('on-exit-leak-free');
const assert = require('assert');

function setup() {
  // This object can be safely garbage collected,
  // and the resulting shutdown function will not be called.
  // There are no leaks.
  const obj = { foo: 'bar' };
  register(obj, shutdown);
  // use registerBeforeExit(obj, shutdown) to execute the function only
  // on beforeExit
  // call unregister(obj) to remove
}

let shutdownCalled = false;

// Please make sure that the function passed to register()
// does not create a closure around unnecessary objects.
function shutdown(obj, eventName) {
  console.log(eventName); // beforeExit
  shutdownCalled = true;
  assert.strictEqual(obj.foo, 'bar');
}

setup();

process.on('exit', function () {
  assert.strictEqual(shutdownCalled, true);
});

var BYTES_IN_MB = 1024 * 1024; // 1MB = 1024KB = 1024(KB/MB)*1024(B/KB)
var BYTES_IN_SMI = 4;
var NUM_SMIS_IN_MB = BYTES_IN_MB / BYTES_IN_SMI;

function allocateMB() {
  var y = [];
  for (var i = 0; i < NUM_SMIS_IN_MB; i++) {
    y.push(Math.random());
  }
  return y;
}

const reg = new FinalizationRegistry((heldValue) => {
  console.log(true);
});

setInterval(() => {
  register(allocateMB(), shutdown);
}, 1000);

[jimmywarting]

yea, the large array seems to be GC... that's b/c you are using weakref instead...
but the weakref themself seems to be saved into refs?

on-exit-leak-free/index.js

Lines 3 to 6 in 223fd00

	const refs = {
	exit: [],
	beforeExit: []
	}

so the clean up (shutdown) function is never called up until the process exist?

[mcollina]

The WeakRefs are cleaned up in

on-exit-leak-free/index.js

Lines 50 to 56 in 223fd00

	function clear (ref) {
	for (const event of ['exit', 'beforeExit']) {
	const index = refs[event].indexOf(ref)
	refs[event].splice(index, index + 1)
	uninstall(event)
	}
	}

when the FinalizationRegistry calls his callback.

[jimmywarting]

tried something more scaled down...

const { register, unregister } = require('on-exit-leak-free');
const reg = new FinalizationRegistry(clear);

function clear() {
  console.log('running clear');
}

function shutdown() {
  console.log('running shutdown');
}

function allocateMB() {
  return Array(1024)
    .fill(0)
    .map((e) => Math.random());
}

setInterval(() => {
  register(allocateMB(), shutdown);
  reg.register(allocateMB(), 'xyz');
}, 10);

This are doing the same stuff more or less... except that my manual approach calls clear a few times and on-exit-leak dose not call shutdown

[mcollina]

It's not calling shutdown if the objects have been collected. That's a feature of this module, not a bug.

[jimmywarting]

Hmm, okey... then i'm not really sure if i can use this...
i do need to know when when a blob have been GC so that i can go ahead and delete it from the the temp file system...

[mcollina]

This looks like a feature we should support.

How would you see the API look like? How would you know what file to delete on disk if the object you are receiving has been collected and it's know undefined. Should we add some metadata?

[jimmywarting]

It felt a bit unconventional to see something using the object itself along with finalizers
your example is basically

function shutdown (obj, eventName) {
  // here you are using the obj itself that you registered
  obj.foo === 'bar'
}
const obj = { foo: 'bar' }
register(obj, shutdown)

This is the first for me...

From every occasion i have stumble up on some code examples that uses FinalizationRegistry (and they are very rare) then it have mostly been with the help of the heldValue as a metadata value

How would you know what file to delete on disk if the object you are receiving has been collected and it's know undefined

So the way I would go about using finalizers myself would be to do something like:

const registry = new FinalizationRegistry(filePathToDelete => {
  // i would never use the blob itself in here
  unlink(filePathToDelete)
})

const filePathToDelete = './package.json'
registry(blob, filePathToDelete)

So i think it's a bit weird what you are doing... but hey it seems to be what this package is all about.

same thing if i where to listening on when some user got GC and i needed to know who got erased then i would not create a function that passes in the hole user object... i would have just used the user id as metaData / heldValue

const registry = new FinalizationRegistry(userId => {
  // A user got GC
})
const userId = user.id
registry(user, userId)

if i wanted the hole object then i would have used a clone registry(user, { ...user })

[jimmywarting]

I don't have any suggestion right now.
but if i would have written this package from scratch then the public api would be a bit different...

I would maybe just written it like this:

var heldValue = 'bar'
function shutdown (heldValue) {
  console.assert(heldValue === 'bar')
}
const obj = { foo: heldValue }
register(obj, obj.foo, shutdown)

// or if `heldValue` was optional: make it more like setTimeout paramN
register(obj, shutdown, ...args)
function shutdown (...args) { }

and do all the process listening underneath, would maybe totally omit what kind of event it is also...

[mcollina]

Great idea, I'll refactor and add some of this. I'm happy to bump the major as I control the main dependant of this module (pino).

[jimmywarting]

I was thinking about this a little more about what you have built with on-exit-leak-free and i do think it would be useful if NodeJS kind of had this built in... and perhaps somewhat diverged from the specification and allowed us to write something like:

new FinalizationRegistry(fn, {
  runOnExit: true
})

Or to confront with browser naming convention: runOnUnload that runs while onbeforeunload event is running

Also To quote MDN:

A conforming JavaScript implementation, even one that does garbage collection, is not required to call cleanup callbacks. When and whether it does so is entirely down to the implementation of the JavaScript engine.

Would it be insane if NodeJS first did garbage collection first, then called any registered finalizaor, and then quited the process?

Either way i think i want to make this proposal to the web as well:

const reg = new FinalizationRegistry(clear)
reg.register(obj, id, { runOnUnload: true })

// or
const reg = new FinalizationRegistry(clear, { runOnUnload: true }) 

[mcollina]

I think this is fundamentally a missing feature of Node.js and the web, I agree.

I'll open an issue on adding this feature to Node.js itself.

[jimmywarting]

I'll open an issue on adding this feature to Node.js itself.

Did you ever create a a github issue in to Node.js?
would be grate to link it to this issue