rzo1 commented on PR #2940: URL: https://github.com/apache/tomee/pull/2940#issuecomment-5623050207
@JorrenH Good question. For the paths this PR changes, no. `capture()` runs on the thread that owns the context, `begin()` builds a new `ThreadContext` per task on the thread running it, and `SecurityThreadContextProvider` only copies the context that was just entered on that same thread. So the concurrency utilities no longer touch another thread's `ThreadContext`. I'd still keep the `synchronized` blocks for now: - `data` is still a `Collections.synchronizedMap`, and its contract requires holding the map's monitor while iterating. Dropping the blocks but keeping the map would just be incorrect in a different way. - `ThreadContext` is public, and references to it escape in other places. For example, `PoolEndpointHandler` keeps the creating thread's context in a field, and `RequestScopedThreadContextListener.DestroyContext` holds one. I haven't checked all of those, so I can't say yet that it is truly confined to its thread. - A monitor nobody else is holding costs almost nothing, and the maps are small. Once we've checked that nothing hands a live `ThreadContext` to another thread, we could replace the `synchronizedMap` with a plain `HashMap` in a follow-up issue. That's a bigger change than this PR should make, though. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
