Introduce caching of ResteasyUriInfo.InitData #4563
Conversation
geoand
force-pushed
the
resteasyuriinfo-cache
branch
from
6c832bd
to
653e617
Compare
| @@ -114,6 +136,19 @@ private void dispatch(RoutingContext routingContext, InputStream is, VertxOutput | |||
| if (!vertxRequest.getAsyncContext().isSuspended()) { | |||
| try { | |||
| vertxResponse.finish(); | |||
| // the reason we only cache successful responses is to ensure that a torrent of erroneous URLs | |||
| // doesn't fill up the cache and cause a DoS | |||
There was a problem hiding this comment.
I think you should cache unsuccesfull responses as well, especially if we go for having a proper cache.
DDoS are caused by the system being overwhelmed; hitting the "bad response cache" would be the best way to avoid it as the cache would help fighting load.
There was a problem hiding this comment.
I agree 💯%. If we have a proper cache than all this nastiness can go away.
| // we don't want the cache to grow unbounded since values path params could potentially be infinite | ||
| // and if left unchecked would take up all memory if the application is up for long enough | ||
| if (resteasyUriInfoInitDataMap.size() > resteasyUriInfoCacheMaxSize) { | ||
| resteasyUriInfoInitDataMap.clear(); // this is super lame and should probably be revisited |
There was a problem hiding this comment.
yes it's dangerous. See my other comments on the design discussion.
There was a problem hiding this comment.
If we have the RESTEasy part of the PR in which puts in the necessary hooks, we can absolutely improve this based on what you mention about the existence or not of Caffeine.
Note that the way I have done the RESTEasy PR (and as of course you have seen from looking at this PR), the caching itself is handled exclusively in Quarkus so we can probably get as creative as we like.
...steasy/runtime/src/main/java/io/quarkus/resteasy/runtime/standalone/VertxRequestHandler.java
Outdated
Show resolved
Hide resolved
geoand
force-pushed
the
resteasyuriinfo-cache
branch
2 times, most recently
from
22e607a
to
74a1e1f
Compare
|
@geoand what's the status of this? I guess it might be too late but I'd still love to get some improvements in this area. |
|
@Sanne I can rebase onto |
|
I didn't do it so far because this wasn't critical, but it won't be a problem to update it (it's a temporary solution anyway until we have a better "cache") |
|
@geoand yes please do :) And ping me for review, I can quickly run some perf tests. |
geoand
force-pushed
the
resteasyuriinfo-cache
branch
from
74a1e1f
to
906d284
Compare
|
@Sanne can you give it a shot and see if it gives you any significant improvement? |
geoand
force-pushed
the
resteasyuriinfo-cache
branch
from
906d284
to
c74f9bc
Compare
|
I'm cancelling the build for this one. |
|
Sure, no problem |
|
@geoand my results are being puzzling so far. I'll need to repeat the runs and gather some more data. |
|
OK, thanks for keeping me up to date |
|
@Sanne were you able to see if this change actually had much of an effect? |
|
It actually made performance a little worse :-) But I haven't been able to check why though... I will try diagnosing this today, as the patch looks sensible but maybe we're missing something. |
|
Interesting! If you find anything let me know so we can see how we can improve the situation. Thanks! |
|
Sure, I will. We need some luck for me to be able to focus on this long enough though, I'm using it as a "fun task" to do between priority tasks ;) |
|
I see :) |
|
@geoand Is this PR still being worked on or can it be safely closed? |
|
Let's close it |
This is done because the computation of the data is expensive
Relates to: #4345
Depends on: resteasy/resteasy#2184Depends on: RESTEasy4.4.0.Final(still unreleased)