Closed jllanes3 closed 1 year ago
pjfanning
commented
1 year ago @cowtowncoder since jackson v2.14 and the changes in LRUMap to use a ConcurrentLinkedHashMap implementation, the synchronization around _writers and _readers in ProviderBase may not be needed.
cowtowncoder
commented
1 year ago @pjfanning You are absolutely right -- since implementation is synchronized, sync here should not be needed (and ditto for jackson-jakarta-rs-providers).
cowtowncoder
commented
1 year ago Ok, fixed via #167, to be included in 2.14.2 / 2.15.0.
jllanes3
commented
1 year ago Thanks for fixing this super fast. Do you know when we could expect to see 2.14.2 out? I really need the fix, and I want to plan around the release date if possible.
cowtowncoder
commented
1 year ago I am hoping to get 2.14.2 released in couple of weeks; might be very soon but cannot promise due to fluctuating time I have: each full patch release takes 2.5-3 hours of my time.
e-hubert-opti
commented
6 months ago @cowtowncoder I think the assumption the used LRUMap to be thread-safe because being backed by a concurrent implementation was not correct here and causes unbound growth of this map. Could it be that you confused com.fasterxml.jackson.jaxrs.util (used by ProviderBase) with com.fasterxml.jackson.databind.util.LRUMap? Am going to file a bug against Jersey 2.41, but it looks like they only ported this change into their code base. So likely someone should also file an issue with Jackson.
cowtowncoder
commented
6 months ago @e-hubert-opti Whoa! You are right: I had forgotten about copy-pasted version of LRUMap. Yes, I think we need to change JAX-RS/Jakarta-RS providers to use jackson-databind version, which was rewritten in 2.14.
cowtowncoder
commented
6 months ago e-hubert-opti
commented
6 months ago Thank you very much for this ultra fast response @cowtowncoder. I adjusted my unit test to validate the behavior of the LRUMap implementation and changed the import to com.fasterxml.jackson.databind.util.LRUMap (version 2.16.0) and it also fails:
@Test
void test_concurrent_usage_of_lru_map() {
LookupCache<Integer, Integer> lruMap = new LRUMap<>(16, 120);
ExecutorService executorService = Executors.newFixedThreadPool(10);
for (int i = 0; i < 100000; i++) {
int tmp = i;
executorService.submit(() -> lruMap.put(tmp, tmp));
}
assertThat(lruMap.size()).isLessThanOrEqualTo(120);
}java.lang.AssertionError: Expecting actual: 5242 to be less than or equal to: 120
Synchronizing the map with Collections.synchronizedMap() turns it green. I will try to have a closer look at the LRUMap implementation of jackson-databind, but wanted to quickly provide feedback.
e-hubert-opti
commented
6 months ago Ah, all good. I had a look in the implementation of PrivateMaxEntriesMap and noticed the async draining, so it is just the test which needs to be adjusted for this.
cowtowncoder
commented
6 months ago Yeah older versions of LRUMap suffered from various problems; original (one copied here) was not thread-safe wrt mutations; next variation was safe, had flush but no actual LRU logic. Latest version is bit of an overkill, but should work correctly and efficiently, even if not very memory efficiently.
Libraries I am using:
I am seeing thread contention on the following line of ProviderBase.java.
Thread that locked the _writers instance:
Thread (1 of many!) that is waiting for such lock:
JMC for a 1 min JFR:
Full code:
Are you aware if this is expected say when moving to Dropwizard async request handling (i.e. using AsyncResponder.resume(...))?
If this is by design, are you aware of any workarounds when using Dropwizard + Jersey + Jackson (in async mode) that could help with this contention?