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

Introduce system property for controlling URI validation #25092

Merged
merged 1 commit into from
Apr 23, 2022

Conversation

geoand
Copy link
Contributor

@geoand geoand commented Apr 22, 2022

This is not meant to be used in production and is mainly
aimed at benchmarking.
The property is very much analogous to what Vert.x provides
with vertx.disableHttpHeadersValidation

cc @Sanne @johnaohara

Copy link
Member

@Sanne Sanne left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Interesting :)
Might be relevant for some work @franz1981 is doing today

@gastaldi gastaldi added the triage/waiting-for-ci Ready to merge when CI successfully finishes label Apr 22, 2022
* Constant that disables HTTP headers validation, this is a constant so the JIT can eliminate validation code
* and save some allocations.
*/
private final boolean DISABLE_URI_VALIDATION = Boolean.getBoolean(DISABLE_URI_VALIDATION_PROP_NAME);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

must be static too in order to be treated as constant

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍🏻 Good catch!

Copy link
Contributor Author

@geoand geoand Apr 22, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought of that, but decided against it so as to not have it captured during native image build.

C2 will make it a constant in any case no?

I'm likely being too paranoid about the native build thing, it's very unlikely someone would set this at build time (as it's undocumented)

Copy link
Contributor

@franz1981 franz1981 Apr 22, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense, but still the comment

this is a constant so the JIT can eliminate validation code and save some allocations.

is not valid anymore
Netty is using many system properties like these and I expect them to be correctly handled on native image (which knowledge I'm still lacking) - is it not the same? (newbie question :) )

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The constants we care about to not be captured in native image, we explicitly "reset" using a substitution.

We could of course do that here too, but it feels like too much work for such a niche feature

Copy link
Contributor

@franz1981 franz1981 Apr 22, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

At this point let's treat this as a (non-JIT) final field that save some useless work (that's fine) - but at least I suggest to read it somewhere (a static somewhere) in order to save the sys prop to be read each time (unless we care about it changing over time) for each instance creation

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point, however the handler is only created once :).

I did push an update to the comment

@gastaldi gastaldi added triage/waiting-for-ci Ready to merge when CI successfully finishes and removed triage/waiting-for-ci Ready to merge when CI successfully finishes labels Apr 22, 2022
This is not meant to be used in production and is mainly
aimed at benchmarking.
The property is very much analogous to what Vert.x provides
with vertx.disableHttpHeadersValidation
@geoand geoand merged commit 93d33b2 into quarkusio:main Apr 23, 2022
@quarkus-bot quarkus-bot bot added this to the 2.9 - main milestone Apr 23, 2022
@quarkus-bot quarkus-bot bot removed the triage/waiting-for-ci Ready to merge when CI successfully finishes label Apr 23, 2022
@geoand geoand deleted the uri-validation branch April 25, 2022 07:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants