Skip to content
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

Extreme slowdown after upgrading to version 2.4.0 #224

Closed
kiejo opened this issue Nov 2, 2018 · 10 comments
Closed

Extreme slowdown after upgrading to version 2.4.0 #224

kiejo opened this issue Nov 2, 2018 · 10 comments
Labels
deployment issues related to deployment, server setup discussion enhancement help wanted

Comments

@kiejo
Copy link
Contributor

kiejo commented Nov 2, 2018

Hi,
I'm currently in the process of upgrading from version 2.3.7 to 2.4.0, but my SSO related tests now run in 16 seconds vs less than a second before. I saw that there were some large changes in the latest release and Java was added as a dependency, but is such an extreme slowdown expected or is there something I can do to fix it?

@jonathanperret
Copy link

The Java dependency is a showstopper for us for deployment reasons. In any case, I doubt the performance of validation can improve much considering you're spawning an external process to process each SAML login.

Maybe something like https://www.npmjs.com/package/cxsd (or anything that generates plain JS from XSD) could be worth investigating?

@D-32
Copy link

D-32 commented Nov 5, 2018

@jonathanperret the Java dependency was a problem for me too, although it was just javac (compiler) that isn't available on my server. If you can run java, just not javac, using my fork might help. I basically just provide a compiled variant of the java code inside node-xsd-schema-validator (also had to fork that repo).

@jonathanperret
Copy link

@D-32 It's true that the first block we encountered was the lack of javac in our CI environment, so your approach does work around that issue. But the requirement for java is still very annoying, and the performance issue is a blocker for us.

@tngan
Copy link
Owner

tngan commented Nov 5, 2018

@D-32 @jonathanperret

I am thinking to make the validation process to be pluggable. Previously, we are using libxmljs, but it turns out different compile issues among different deployment environment.

@tngan tngan added enhancement deployment issues related to deployment, server setup help wanted discussion labels Nov 5, 2018
@kiejo
Copy link
Contributor Author

kiejo commented Nov 5, 2018

Thanks @jonathanperret for providing more context. It definitely sounds like "spawning an external process to process each SAML login" is what's causing the performance degradation.

Making the validation process pluggable sounds like it could help solve the performance issue if it enables users to use libxmljs as before.

Overall I'm not sure if having such bad performance is a good default for the library as I can imagine this being a blocker for many projects. Unfortunately I don't know about the specific issues the previous solution caused and I assume there were good reasons to move to the Java based approach.

@tngan
Copy link
Owner

tngan commented Nov 5, 2018

@kiejo You may also want to checkout this discussion thread. #216

@jonathanperret
Copy link

@tngan Yes, I saw #216 before commenting here. I understand you had trouble with libxmljs building. In fact we tried to roll back to [email protected] and it failed on one of our Macs because of nodejs/node-gyp#1454 .

In my opinion, libxmljs is still a better choice than depending on Java. You may want to take a look at versions 0.18.x of libxmljs[-mt] where they have added support for prebuilt binaries, which means that most people shouldn't have to compile it (it fixed the build problem we had, unfortunately [email protected] depends on [email protected]).

For the moment we have forked samlify and removed validation altogether (https://github.com/1024pix/samlify/tree/no-xml-validation). Before going to production we are considering switching to https://github.com/bergie/passport-saml which despite having an unwanted passport dependency (see node-saml/passport-saml#295) does not depend on an external validation library.

I hope this validation issue can be fixed because apart from that samlify fits our need perfectly.

jonathanperret added a commit to 1024pix/pix that referenced this issue Nov 21, 2018
The current version of samlify (2.4.x) has a dependency on a
Java environment for XML validation. This creates bad performance
issues (tngan/samlify#224) and makes
CI and deployment complicated.

For now, we are using our own fork of samlify that skips XML
validation altogether.
@tngan
Copy link
Owner

tngan commented Jan 11, 2019

#236

Some update, the module for pluggable validator is in progress. :)

@tngan
Copy link
Owner

tngan commented Jan 12, 2019

@jonathanperret @kiejo

With v2.5.0-rc1, now you can choose either javac or libxml to be the schema validator.

@tngan
Copy link
Owner

tngan commented Jan 13, 2019

I will close it first since there is an option to get rid of this issue from external schema validator.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
deployment issues related to deployment, server setup discussion enhancement help wanted
Projects
None yet
Development

No branches or pull requests

4 participants