-
-
Notifications
You must be signed in to change notification settings - Fork 218
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix: update validator to use samlify-validator-js #510
Conversation
@authenio/samlify-node-xmllint has a built in memory leak every time you run the SAML schema validator. It was related to a library it depends on, node-xmllint, where the source code does not exist anymore. I have therefore create a new library called "samlify-validator-js", building on the existing work, and removed the memory leak.
@ivarconr would you like to further explain what you have fixed for the memory leak? the file you have modified is the compiled file, so it's also difficult for others to cross-check the code change. thanks for your work! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please check the above comment
Yes, I carefully copied of the To identify the the memoery leak I took heap Snapshots from our application and identified that the leak was associated with event listeners being attached to the node process. It does not make any sense for that module to register itself as an event listener on the node process itself. It does this for every execution of the SAML validation. To highlight the fixes performed I isolated that in its own commit. The change is not possible inspect in the GitHub UI, because of the nature of the compiled JavaScript file being to large. I simply removed two lines:
Also highlighted in the IDE diff: This was the easiest way to fix the problem, and allowed us to not have a memory leak in our application. It is not an option for us to rely on any third party tools in Java or C++, as it increases the complexity of self-hosting Unleash. Future improvements: fyi; We are already using this in Unleash Cloud (https://www.getunleash.io/). |
@ivarconr Thank you for your work, I have created Since the source code of xmllint was removed, I will stop the maintenance of the validator, xmllint.js is a compiled script which I was not aware of before. |
This is definitely the way to go. Removing hard dependencies on Java is a good thing for this project. CPU is usually less of a concern (given its not crazy and you have a decent amount of sign-in requests per second). |
I looked in to you code. My biggest concern is the dependency on from: https://github.com/noppa/xmllint-wasm#installation
It might be ok, but requires better understanding of potential side-effects. Any thoughts @tngan? |
@authenio/samlify-node-xmllint has a built in memory leak every time you run the SAML schema validator. It was related to a library it depends on, node-xmllint, where the source code does not exist anymore.
I have therefore create a new library called "samlify-validator-js", building on the existing work, and removed the memory leak.