Fix memory leak when using multiple webpack instances

[valerio]

Closes #1203

This PR should fix a memory leak issue when using multiple webpack instances that get discarded over time.

I initially converted the webpackInstances array to a WeakMap (plus a global index) but that wasn't enough to fix the leak, since the cached TSInstance held in instances.ts was still retaining the webpack compiler and never being freed.

In the end, I removed the webpack instance array and changed the TS instance cache by using a combined WeakMap + Map so that we maintain the same behaviour but keep the lifetime of TSInstances tied to the webpack compiler that spawned them.

There is one small change in how loaderOptions.instance names are generated:

• old names: 0_default_e3b0c44298fc1c14
• new names: default_e3b0c44298fc1c14

They now don't contain the unique index for the webpack compiler that generated them, this shouldn't change the way instances are cached, since they are now tied to a specific webpack instance by reference (the key of the WeakMap) instead of using the index.

I've tested that the leak is fixed using this repo: https://github.com/valerio/ts-loader-leak
Memory now gets periodically freed between runs:

➜  ts-loader-leak git:(main) node index.js 1
webpack will run, process should run out of memory
current heap usage { heapMBs: 101 }
current heap usage { heapMBs: 151 }
// omitted for brevity
current heap usage { heapMBs: 882 }
current heap usage { heapMBs: 937 }
current heap usage { heapMBs: 991 }
current heap usage { heapMBs: 98 } <-- memory was freed 
current heap usage { heapMBs: 154 }
current heap usage { heapMBs: 208 }
current heap usage { heapMBs: 257 }
current heap usage { heapMBs: 312 }
current heap usage { heapMBs: 366 }
current heap usage { heapMBs: 155 } <-- memory was freed 
current heap usage { heapMBs: 202 }
current heap usage { heapMBs: 257 }
current heap usage { heapMBs: 312 }
current heap usage { heapMBs: 359 }

I thought about adding a test case in here but I don't think it would fit with the current setup, it also might be non-deterministic.

[johnnyreilly]

This is fantastic work! Do you want to update the package.json to a new version and add an entry to the CHANGELOG.md? Let's ship this!

[valerio]

This is fantastic work! Do you want to update the package.json to a new version and add an entry to the CHANGELOG.md? Let's ship this!

Done and done! 🚀
And thanks for the speedy review! 😄

[liorcode]

Seems this change breaks my build:

        Module build failed (from ../node_modules/sled-test-runner/node_modules/thread-loader/dist/cjs.js):
        Thread Loader (Worker 2)
        Invalid value used as weak map key
            at PoolWorker.fromErrorObj (node_modules/sled-test-runner/node_modules/thread-loader/dist/WorkerPool.js:262:12)
            at node_modules/sled-test-runner/node_modules/thread-loader/dist/WorkerPool.js:204:29
            at WeakMap.set (<anonymous>)
            at Object.getTypeScriptInstance (node_modules/ts-loader/dist/instances.js:37:23)
            at Object.loader (node_modules/ts-loader/dist/index.js:16:41)  

Had to pin the ts-loader version to 8.0.7 to work around it

[johnnyreilly]

Oh no! @valerio would you be able to take a look please?

@liorcode is it ThreadLoader being in the mix that causes the issue? If you remove ThreadLoader does that fix the issue? (side note; you might find your build is faster without ThreadLoader: https://blog.johnnyreilly.com/2018/12/you-might-not-need-thread-loader.html )

If you could share a minimal reproduction repo that would be helpful

[liorcode]

I am actually using a 3rd party service which itself uses thread loader, so unfortunately I can't control it, but thanks!

[valerio]

        Module build failed (from ../node_modules/sled-test-runner/node_modules/thread-loader/dist/cjs.js):
        Thread Loader (Worker 2)
        Invalid value used as weak map key
            at PoolWorker.fromErrorObj (node_modules/sled-test-runner/node_modules/thread-loader/dist/WorkerPool.js:262:12)
            at node_modules/sled-test-runner/node_modules/thread-loader/dist/WorkerPool.js:204:29
            at WeakMap.set (<anonymous>)
            at Object.getTypeScriptInstance (node_modules/ts-loader/dist/instances.js:37:23)
            at Object.loader (node_modules/ts-loader/dist/index.js:16:41)  

That's a strange issue indeed, some values like Symbol or string can't be used as a weak map key, maybe ThreadLoader is hooking into webpack and replacing the compiler instance with something else?

I'll see if I can reproduce, @liorcode a reproduction repo would definitely help if you have an easy way to set that up!

[Krasnopir]

Have same problems after update.

ERROR in ./apps/admin-client/src/index.tsx
Module build failed (from ./node_modules/thread-loader/dist/cjs.js):
Thread Loader (Worker 0)

Invalid value used as weak map key
at PoolWorker.fromErrorObj (/usr/src/app/node_modules/thread-loader/dist/WorkerPool.js:344:12)
at WeakMap.set (<anonymous>)
at Object.getTypeScriptInstance (/usr/src/app/node_modules/ts-loader/dist/instances.js:37:23)
at Object.loader (/usr/src/app/node_modules/ts-loader/dist/index.js:16:41)

---

my webpack config

        {
          test: /\.tsx?$/,
          exclude: [/node_modules/],
          use: [
            { loader: 'cache-loader' },
            {
              loader: 'thread-loader',
              options: {
                workers: require('os').cpus().length - 1,
                poolTimeout: isProduction ? 500 : Infinity,
              },
            },
            {
              loader: 'ts-loader',
              options: {
                happyPackMode: true, 
              },
            },
          ],
        },

[valerio]

I've managed to reproduce this, opened an issue here: #1206

I'll see if I can come up with a fix for this as well!