Adding safeguard for polyfill of HTMLSelectElement.prototype.remove to avoid memory leaks #48
Conversation
…o avoid memory leaks.
I am sorry to hear you had issues with this library, but your safeguard added also after the check so that it's forced worldwide doesn't seem an answer to me. It wouldn't be the first time people assume my libraries are to blame, and while I am sure you did investigate on this, I am not fully sure what is exactly the issue or what's causing the leak. If you could provide me details about what you think leaks and why, I'd be happy to better fix it for everyone. |
|
Dear Andrea, thank you for the prompt response and your willingness to assist. I can imagine that with such a core popular library there are often reports similar to mine. I also agree there must be a better way to resolve this issue. However, I can say with confidence that the issue is related to dom4. The problem here is specific to the combination with jest. The provided repository link has a clear example. The problem is that jest clears the require cache in between the execution of each test file. It cleans up the jsdom environment, but it does not clean up the react dom primitives (HTML constructors in this case). Each time jest requires dom4 (once per testfile), dom4 wraps the remove function of HTMLSelectElement. With each new testfile HTMLSelectElement grows and grows. But as a result, it also retains references to prototypes and constructors of older test executions and does not allow proper memory clean up. The fundamental issue is that line 232 in src/dom.js. HTMLSelectElement.prototype.remove new function definition contains the original remove function within its scope. Which causes subsequent requires of dom4 to keep nesting .remove. Ideally, the overwrite of HTMLSelectElement.prototype.remove would happen (as mentioned in the original issue) because there's some missing functionality. Or, the overwrite of remove would use a polyfill function independent of the original remove function. This is not a problem in most environments because require caches modules. Jest however specifically overwrites this behaviour to allow the module specifying mocking that jest is so famous for. I hope this additional information provides the context you are searching for. I also strongly advise you check the attached problem which has a very isolated clear representation of the problem. |
No, because nobody includes multiple time a polyfill, there's no reason to do so, and I wonder why that's the case in your test env. |
|
P.S. yes, I have found and understood the issue, and I have a better solution, yet I don't understand why you would include multiple times this script. What is the purpose of having any script included more than once in any project out there these days? |
|
OK, I have understood why you have an issue, jest runs each second test tainted from the previous one. While I'd say "good luck with that" in general, since it's the same problem of a teardown that doesn't really cleanup the setup, I'll patch this in a different way, taking your patch as example. |
|
|
|
Dear Andrea, thank you for your hard work. I certainly did not expect the issue to be resolved so quickly and cleanly! Thank you very very much! Radu |
I work for fleetster (fleetster.de) and we use in our tech stack React, Jest and Blueprintjs.
As the size of our automated tests grew we encountered an unfortunate issue in our gitlab runners. We were hitting the memory limit of the JS heap (1.4gb).
After quite some diving down the rabbit hole, the true source appears to be an old fix (2015) from the dom4 library.
A simple obvious example of this can be found here:
https://gitlab.com/creosteanu/jest-blueprint-leak
All I have done this is add a safeguard to avoid this problem. There should be a better way to write the remove polyfill which should not require my safeguard.