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

Prevent UOE when MessageBodyReader can handle multiple media types #14604

Merged
merged 1 commit into from Jan 26, 2021

Conversation

geoand
Copy link
Contributor

@geoand geoand commented Jan 26, 2021

The basic idea of the PR is to get rid of the useless usages of MediaTypeHelper#getBestMatch.
Because of that we can now enforce the immutability of the media types in
ResourceReader and ResourceWriter. This change is also beneficial from a performance standpoint.
The places that do indeed need getBestMatch are very unlikely to be on the hot path, so I went ahead and made that method use a copy of the list and operate on it.

Furthermore the PR applies some extra polish to MediaTypeHelper and
does away with the Iterator allocation in during the lookup
of the appropriate MessageBodyReader and MessageBodyWriter classes

Related to: #13997 (comment)

@gsmet
Copy link
Member

gsmet commented Jan 26, 2021

It might be a stupid idea but why don't you sort them once and for all before making the list immutable?

Or do you expect the list to be sorted in different ways depending on the context?

@geoand
Copy link
Contributor Author

geoand commented Jan 26, 2021

It might be a stupid idea but why don't you sort them once and for all before making the list immutable?

Although this makes sense, it won't work because the ResourceReaderComparator needs to compare the original media types (basically it does this because these are taken as the declaration of how imporant the MessageBodyReader to that media type - if that explanation makes any sense)

@geoand
Copy link
Contributor Author

geoand commented Jan 26, 2021

But this indeed warrants another look into how the readers are determined (although I really hate this part as there are so many TCK tests that fail even when the slightest change is made...)

@geoand geoand changed the title Prevent ISE when MessageBodyReader can handle multiple media types Prevent UOE when MessageBodyReader can handle multiple media types Jan 26, 2021
The basic idea of the PR is to get rid of the useless usages of MediaTypeHelper.getBestMatch.
Because of that we can now enforce the immutability of the media types in
ResourceReader and ResourceWriter.

Furthermore the PR applies some extra polish to MediaTypeHelper and
does away with the Iterator allocation in during the lookup
of the appropriate MessageBodyReader and MessageBodyWriter classes

Related to: quarkusio#13997 (comment)
@geoand
Copy link
Contributor Author

geoand commented Jan 26, 2021

I totally changed the PR and also updated the description.

@geoand geoand requested a review from FroMage January 26, 2021 12:02
if (!hasAtMostOneItem(provided)) {
provided = new ArrayList<>(provided);
sortByWeight(provided);
}
Copy link
Member

Choose a reason for hiding this comment

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

This is never used in a hot path? Because I could see how the extra allocations could slow things down.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is not, because I removed the calls to getBestMatch from the hot path. The remaining calls to it are obscure features

@geoand geoand added the triage/waiting-for-ci Ready to merge when CI successfully finishes label Jan 26, 2021
@gsmet gsmet merged commit c73723e into quarkusio:master Jan 26, 2021
@ghost ghost added this to the 1.12 - master milestone Jan 26, 2021
@gsmet gsmet modified the milestones: 1.12 - master, 1.11.1.Final Jan 26, 2021
@geoand geoand deleted the rr-illegal-sort branch January 26, 2021 16:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/resteasy-reactive triage/waiting-for-ci Ready to merge when CI successfully finishes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants