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

[RESTEASY-2646]:Fix match cache in RootNode grows infinitely #2471

Merged
merged 7 commits into from
Jul 17, 2020

Conversation

jimma
Copy link
Contributor

@jimma jimma commented Jul 13, 2020

No description provided.

@@ -56,10 +69,12 @@ public ResourceInvoker match(HttpRequest request, int start)
request.setAttribute(RESTEASY_CHOSEN_ACCEPT, match.chosen);
} else {
match = root.match(request, start);
if (match.match != null && match.match.expression.getNumGroups() == 0 && match.invoker instanceof ResourceMethodInvoker) {
if (cache.size() < CACHE_SIZE && match.match != null && match.match.expression.getNumGroups() == 0 && match.invoker instanceof ResourceMethodInvoker) {
Copy link
Member

Choose a reason for hiding this comment

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

So once the cache is full, nothing gets added / old entries removed?

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's ideal that we remove the LRU entry when the cache is full. Later we can look at ConcurrentLinkedHashMap which Ben suggested.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Just remember, that 99% of apps will only have a handful of media types (< 5), usually only 1 (json). Might be a little bigger if character sets are involved, but there's only a handful of those in existence as well. The issue here was a media type that adds a random parameter. IMO, if you hit the cache size you're dealing with an app that has another random parameter and the cache will be useless. Might as well clear it and turn it off in that scenario.

Choose a reason for hiding this comment

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

Isn't this per route? The cache size would be hit if an application has more than CACHE_SIZE routes, in the expected case.

The MediaTypeHeaderDelegate is a cache of media types and it clears itself when full. It doesn't condition for dynamic parameters, so a multipart route would invalidate it regularly.

@rsvoboda
Copy link
Member

@jimma Do you plan to include test case?

@jimma
Copy link
Contributor Author

jimma commented Jul 14, 2020

@rsvoboda I didn't figure out how to write a test case for this issue. Do you have any idea ?

@rsvoboda
Copy link
Member

@jimma nothing simple what could be added into the RESTEasy testsuite.
Maybe just small check on cache size or several calls for public ResourceInvoker match(HttpRequest request, int start). But it would probably need to involve Byteman

@rsvoboda
Copy link
Member

You can do this:

    public class MyRootNode extends RootNode {
        public int cacheSize() {
            return cache.size();
        }
    }

and run 2050x myRootNode.match(...) and check that myRootNode.cacheSize() returns 2048.

@ronsigal
Copy link
Collaborator

As for managing overflow, LRU might be the appropriate strategy, but Bill's idea of just zapping the cache and starting over would have a similar effect.

  1. There should be an entry in the User Guide about RESTEASY_MATCH_CACHE_ENABLED
  2. We should add a note about random parameters in the User Guide and the idea of turning off the cache.

@ronsigal
Copy link
Collaborator

I hate to mention this, but as of WF 20, the jaxrs module allows setting RESTEasy parameters. So ... technically ... we should update jaxrs. I HOPE that doesn't count as a Feature.

@rsvoboda
Copy link
Member

@ronsigal WF is using 3.x series - https://github.com/wildfly/wildfly/blob/master/pom.xml#L392
I don't think there is need to do any work on WF side because of this parameter

@rsvoboda
Copy link
Member

I have prepared RootNodeCacheSizeTest rsvoboda@0406ac9 in my https://github.com/rsvoboda/Resteasy/tree/RootNodeCacheSizeTest branch

If you like it, feel free to cherry pick it and include into this PR.

@asoldano
Copy link
Member

I have prepared RootNodeCacheSizeTest rsvoboda@0406ac9 in my https://github.com/rsvoboda/Resteasy/tree/RootNodeCacheSizeTest branch

If you like it, feel free to cherry pick it and include into this PR.

Thanks @rsvoboda . Please @jimma include this.

@asoldano
Copy link
Member

I hate to mention this, but as of WF 20, the jaxrs module allows setting RESTEasy parameters. So ... technically ... we should update jaxrs. I HOPE that doesn't count as a Feature.

As @rsvoboda commented, WildFly is not a concern atm, as this issue affects RESTEasy 4.x series only.

@jimma
Copy link
Contributor Author

jimma commented Jul 15, 2020

As for managing overflow, LRU might be the appropriate strategy, but Bill's idea of just zapping the cache and starting over would have a similar effect.

+1. I already changed to clear cache when it's full and start over.

  1. There should be an entry in the User Guide about RESTEASY_MATCH_CACHE_ENABLED
  2. We should add a note about random parameters in the User Guide and the idea of turning off the cache.

I'll add it to the documentation later.

@jimma
Copy link
Contributor Author

jimma commented Jul 15, 2020

I have prepared RootNodeCacheSizeTest rsvoboda@0406ac9 in my https://github.com/rsvoboda/Resteasy/tree/RootNodeCacheSizeTest branch

If you like it, feel free to cherry pick it and include into this PR.
I already cherry-picked, Thanks. I did some change in the test after I improved something as described in this comment : #2471 (comment) .

@jimma
Copy link
Contributor Author

jimma commented Jul 15, 2020

@jimma Shouldn't be the cache size at this phase equal to 2 (2050-2048)?

oops. Missed one commit change. Fixed.

@michalszynkiewicz
Copy link
Contributor

the issue number sems wrong. Is it for RESTEASY-2643 ?

@rsvoboda
Copy link
Member

the issue number sems wrong. Is it for RESTEASY-2643 ?

both are correct :)

@ronsigal
Copy link
Collaborator

re: "WF is using 3.x series"

D'oh!!

Copy link
Member

@asoldano asoldano left a comment

Choose a reason for hiding this comment

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

This looks ok to me now. Does anybody have strong concerns on moving on with this fix for now and possibly later enhance the solution with a proper caching mechanism / library integration?
/cc @patriot1burke

@ronsigal
Copy link
Collaborator

@asoldano, it's fine with me. I would also add documentation to the TODO list.

@asoldano asoldano merged commit 556fa43 into resteasy:master Jul 17, 2020
@ben-manes
Copy link

Thank you for the prompt fix. Any timeline on when a release will be made?

asoldano pushed a commit to asoldano/Resteasy that referenced this pull request Sep 17, 2020
…y#2471)

* [RESTEASY-2646]:Initial fix

* [RESTEASY-2646]:More improvement

* [RESTEASY-2646]:Get match catch config from system property

* Code cleanup

* [RESTEASY-2646]:Without caching match if the ContentType contains parameters

* Test for RootNode cache size limit

* Fix test

Co-authored-by: Rostislav Svoboda <rsvoboda@redhat.com>
ronsigal added a commit that referenced this pull request Sep 17, 2020
* [RESTEASY-2625]:Fix @produces(text/event-stream

* [RESTEASY-2610] Upgrade MicroProfile REST Client to 1.4.1

* [RESTEASY-2595] Add getLinks method with a custom class loader

* [RESTEASY-2597] add methods and test to support setting

* [RESTEASY-2300] allow empty Host header for netty adapters

* [RESTEASY-2300] test-cases for empty Host header field

* Remove deps from the BOM that don't exist in the current version

* [RESTEASY-2633] Encode code points that span multiple characters (#2456)

Fixes RESTEASY-2633

* [RESTEASY-2637] Fix issue with Spring Web parameters encoding (#2453)

Fixes: quarkusio/quarkus#10133

* Upgrade to wildfly 20.0.0.Final

* Disable wiremock banner

* [RESTEASY-2639] Bump version.log4j from 2.9.1 to 2.13.3 in /resteasy-dependencies-bom (#2460)

Bumps `version.log4j` from 2.9.1 to 2.13.3.

Updates `log4j-api` from 2.9.1 to 2.13.3

Updates `log4j-core` from 2.9.1 to 2.13.3

Signed-off-by: dependabot[bot] <support@github.com>

Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

* [RESTEASY-2634] Fix HttpHeaders.getAcceptableLanguages (#2458)

JAX-RS mandates that HttpHeaders.getAcceptableLanguages() returns a
single wildcard locale instance if no acceptable languages are defined.

* [RESTEASY-2642] Add doPrivs to sys prop checks (#2467)

* [RESTEASY-2642] Add doPrivs to sys prop checks

Signed-off-by: Andy McCright <j.andrew.mccright@gmail.com>

* Review comment: only use doPriv if SecMgr is used

Signed-off-by: Andy McCright <j.andrew.mccright@gmail.com>

* [RESTEASY-2645] Upgrade Jackson to 2.11.1

* RESTEASY-2648 Upgrade json-patch to 1.13

* [RESTEASY-2648][RESTEASY-2327] Additional minor fix to modules and removal of exclusion

* [RESTEASY-2649] Upgrade shrinkwrap to 2.2.7 (#2473)

* [RESTEASY-2646]:Fix match cache in  RootNode grows infinitely (#2471)

* [RESTEASY-2646]:Initial fix

* [RESTEASY-2646]:More improvement

* [RESTEASY-2646]:Get match catch config from system property

* Code cleanup

* [RESTEASY-2646]:Without caching match if the ContentType contains parameters

* Test for RootNode cache size limit

* Fix test

Co-authored-by: Rostislav Svoboda <rsvoboda@redhat.com>

* Test for RESTEASY-2633

* [RESTEASY-2659] Performance improvements (#2479)

- Avoid unnecessary use of URI.create - rather do more lazily
- Multivalued maps will 99% of the time have 1 or maybe 2 entries for a key so don't allocate memory for 10
- Avoid unnecessary matcher creation when decoding when a string does not contain % in it - which is the more likely case for URI's to Rest resources

These changes increase throughput by about 2% on a simple Rest service.

Co-authored-by: Paul Carter-Brown <paul.carter-brown@jini.guru>

* RESTEASY-2662 Don't create a context if it does not exist

* [RESTEASY-2666] Upgrade httpclient/httpcore

* [RESTEASY-2670] Ensure immutability and properly rely on builders

* [RESTEASY-2669] Dealt with a few warnings

* [RESTEASY-2668] Remove SseEventOutputProvider

* [RESTEASY-2667] Removing SerializableProvider

* [RESTEASY-2669] Misc cleanup and warning fixes

* [RESTEASY-2669] Further warnings' cleanup in core

* RESTEASY-2674 Escape sign of quotation to see the value of parameter

* [RESTEASY-2661]:MediaTypeMap shared across threads, but cache is not … (#2498)

* [RESTEASY-2661]:MediaTypeMap shared across threads, but cache is not thread-safe

* Update MediaTypeMap.java

Removing comment about volatility.

Co-authored-by: Ron Sigal <rsigal@redhat.com>

* [RESTEASY-2549] Remove resteasy-validator-provider source dependency on Hibernate Validate.

[RESTEASY-2549] Moved NonCDIValidatorFactoryTest to integration-tests-embedded

* Do not use ResteasyProviderFactory.getInstance() in SseEventProvider

* [RESTEASY-2684] Statically set provider factory on ResteasyClientBuilderImpl (#2504)

* [RESTEASY-2684] Statically set provider factory on ResteasyClientBuilderImpl

* [RESTEASY-2684] Added unit test.

* [RESTEASY-2684] Fix new unit test.

Co-authored-by: Ron Sigal <rsigal@redhat.com>

* [RESTEASY-2683] NPE in ApacheHttpClient43Test because cache is null in MediaTypeMap (#2501)

* [RESTEASY-2592] removed profile forward.compatibility and annotation NotForForwardCompatibility

* RESTEASY-2678 - Fix read priority from @priority with @RegisterProvider.

* [RESTEASY-2687] Revisit fix in previous commit and remove ResteasyProviderFactory.getInstance() in other locations as well

* Deprecated unused classes

* [RESTEASY-2592] Restore unintentially disabled testsuites

* Update target containers

* [RESTEASY-2627] Chain thenCompose properly

* [RESTEASY-2689] Fix SseEventOutputImpl wrong CompletionStage composition sequence

* RESTEASY-2678 - Fix test.

* [RESTEASY-2689]:Add a test case

* [RESTEASY-2685] Merge client headers with MP REST Client

* Add test to verify behavior

* Needed to reset beanManager inside the extension for testing

* Build to 4.5.7-SNAPSHOT

Co-authored-by: Jim Ma <ema@redhat.com>
Co-authored-by: Gytis Trikleris <gytis@redhat.com>
Co-authored-by: R Searls <rsearls@redhat.com>
Co-authored-by: Ivo Studensky <istudens@redhat.com>
Co-authored-by: Alexey Loubyansky <olubyans@redhat.com>
Co-authored-by: Stuart Douglas <stuart.w.douglas@gmail.com>
Co-authored-by: Georgios Andrianakis <geoand@gmail.com>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Co-authored-by: Christoph Böhme <christoph@b3e.net>
Co-authored-by: Andy McCright <j.andrew.mccright@gmail.com>
Co-authored-by: George Gastaldi <gegastaldi@gmail.com>
Co-authored-by: Tomaz Cerar <tomaz.cerar@gmail.com>
Co-authored-by: Rostislav Svoboda <rsvoboda@redhat.com>
Co-authored-by: bcluap <bcluap@gmail.com>
Co-authored-by: Paul Carter-Brown <paul.carter-brown@jini.guru>
Co-authored-by: Ron Sigal <rsigal@redhat.com>
Co-authored-by: Ken Finnigan <ken@kenfinnigan.me>
Co-authored-by: rmartinc <rmartinc@redhat.com>
Co-authored-by: Roberto Cortez <radcortez@yahoo.com>
Co-authored-by: a.koshkin <a.koshkin@tinkoff.ru>
asoldano pushed a commit that referenced this pull request Sep 18, 2020
* [RESTEASY-2646]:Initial fix

* [RESTEASY-2646]:More improvement

* [RESTEASY-2646]:Get match catch config from system property

* Code cleanup

* [RESTEASY-2646]:Without caching match if the ContentType contains parameters

* Test for RootNode cache size limit

* Fix test

Co-authored-by: Rostislav Svoboda <rsvoboda@redhat.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
7 participants