fix: attach queue to fs module instead of global

[SimenB]

Fixed #183. I don't know how to write a test here without basically implementing require within the test.

fs returns the same core module no matter what, so it should deduplicate correctly regardless of importFresh or not

[SimenB]

I've applied this patch via patch-package in Jest's repo, and can confirm it fixes the memory leak

[isaacs]

@coreyfarrell what say you?

[coreyfarrell]

Is it possible to proxyquire or mock the require('fs') done by graceful-fs? That is the only concern I can think of is what happens if someone stubs out some fs functions for graceful-fs, I assume graceful-fs would get a clone of the fs module? If so we could be swapping one memory leak for another.

[SimenB]

If people override require somehow (like proxyquire), then yeah. You already write to fs (to patch close and closeSync), so it seems to me this is already an issue today? I don't know if it's more (or less) GC-able

[coreyfarrell]

So one additional thought for a new 4.x release to put the symbol on fs that would need to be in addition to global, not instead of. An issue is if something bundles or pins to graceful-fs@4.2.3 but something else pulls a future version then they need to share the same gracefulQueue array.

This is a more difficult issue than I initially thought, I will have to think it over a bit more.

I wonder if we could monkey-patch context creation to copy global[gracefulQueue] into the new global variable context? Sorry if this is not a reasonable thought just trying to consider all potential options.

[SimenB]

If it's still attached to global then we're back to where we are today. We could of course try to read it from global, and copy it over to fs if found though, to be nicer to those with multiple copies

[coreyfarrell]

Here's the idea I'm having:

function publishQueue(context, queue) {
  Object.defineProperty(context, gracefulQueue, {
    get: function() {
      return queue
    }
  })
}

// Once time initialization
if (!fs[gracefulQueue]) {
  // This queue can be shared by multiple loaded instances
  var queue = global[gracefulQueue] || []
  publishQueue(fs, queue);

  // ... existing code to patch fs.close / fs.closeSync
}

if (!global[gracefulQueue]) {
  publishQueue(global, fs[gracefulQueue]);
}

There is one "not awesome" situation here. If 4.2.3 is loaded before the code published by this PR it will patch fs.close / fs.closeSync twice. This does however avoid potential infinite patching of those functions. In theory we could have a check that would unpatch 4.2.3 before the "Once time initialization":

if (global[gracefulQueue] && !fs[gracefulQueue]) {
  if (fs.close[previousSymbol]) {
    fs.close = fs.close[previousSymbol]
  }

  if (fs.closeSync[previousSymbol]) {
    fs.closeSync = fs.closeSync[previousSymbol]
  }
}

I'm not sure dealing with this edge case is worth the additional complexity.

[SimenB]

@coreyfarrell if it works I'm all for it! If you can prepare a patch I can apply it in Jest and see if we OOM or not on CI (which we do without this PR)

[coreyfarrell]

@SimenB I've pushed an additional commit to maintain global[gracefulQueue] while copying it and prefering fs[gracefulQueue].

[SimenB]

@coreyfarrell thanks! I've applied that and it still works 🙂 Would love to see this land now 👍

[SimenB]

@isaacs @coreyfarrell ready to land? 😀 Jest currently does gracefullify, and without this patch I'm unable to remove it 😬

[SimenB]

Thank you @coreyfarrell!

[coreyfarrell]

Not a problem sorry for the delay, this is a widespread module so changes face a degree of paranoia.

[SimenB]

Very understandable! If you publish it under next I'd be happy to bump in Jest to use it, if you want some usage of it?

[coreyfarrell]

Very understandable! If you publish it under next I'd be happy to bump in Jest to use it, if you want some usage of it?

Didn't you already test with jest using patch-package?

[SimenB]

Yeah, but that's to test in Jest itself, I meant to publish so others can use it as well. patch-package only affect a local repo - if we wanna float this patch we'd have to vendor the module

[SimenB]

Thanks again @coreyfarrell and @isaacs - I've landed the update in Jest now and removed the gracefulify call 👍