jungm commented on PR #2856: URL: https://github.com/apache/tomee/pull/2856#issuecomment-5144074706
## Review ### Overview The PR optimizes `OpenEJBContextConfig`, which Tomcat invokes during WAR deploy annotation scanning, with three changes: (1) memoizing `File.getCanonicalFile()` and `URLs.toFile(URL)` results in two per-instance `HashMap`s, (2) returning early from `processAnnotationsWebResource()` once every module in `webAppInfo.webAnnotatedClasses` has been processed, and (3) seeding the canonical-file cache with the path from `FileResource.getCanonicalPath()`, which is already canonical. One file, +51/−14, with a plausible profiler-backed motivation (the symlink-fallback loop in `processAnnotationsWebResource()` pushes every class-file URL through `isIncludedIn`, so the same handful of files get canonicalized N×M times). ### Correctness I verified all three changes against the surrounding code and Tomcat 11.0.23 sources; the single-threaded logic is sound: - **Early return** — behavior-identical. Once all `info.name` entries are in `webInfClassesAnnotationsProcessed`, the old loop body was a no-op for every subsequent resource, and that branch never delegates to `super`. The early return even skips a latent NPE (`FileResource.getCanonicalPath()` returns `null` on `IOException`, and `new File(null)` throws — pre-existing, exposure strictly reduced). - **Cache seeding** — valid. Tomcat's `FileResource.getCanonicalPath()` returns `resource.getCanonicalPath()`, so the file is its own canonical form and `putIfAbsent` is correct. - **`urlAsFile`** — `URLs.toFile(new URL(s))` is a pure function of the string, so caching is safe and can never go stale; `MalformedURLException` propagates exactly as before. ### Issues **1. Thread safety — the new maps are reachable from Tomcat's parallel annotation scanning (should fix before merge).** When a context sets `parallelAnnotationScanning="true"`, Tomcat's `ContextConfig.processAnnotationsInParallel()` runs `scanWebXmlFragment` → `processAnnotationsUrl` on the server utility executor across multiple threads. TomEE's override reaches the shared maps via `internalProcessAnnotations` → `isIncludedIn`. Before this PR that path had no shared mutable state (`webInfClassesAnnotationsProcessed` is only touched from `processAnnotationsWebResource`, which runs single-threaded in `processClasses`), so the PR introduces the first concurrent mutation hazard here. Concurrent `HashMap.put` can lose entries or corrupt the table during resize. The fix is cheap: make both fields `ConcurrentHashMap`. The get-then-put in `canonical()` stays fine as a benign race since the computed values are idempotent. **2. Caches are never cleared — stale entries across context restart, plus retained memory.** `configureStop()` clears `webInfClassesAnnotationsProcessed` precisely because this object is a lifecycle listener registered once per `StandardContext` (`TomcatWebAppBuilder`) and survives stop/start (reload). Two consequences: - `canonicalFiles` depends on filesystem state (symlinks, file existence). After a redeploy of the same context the cached resolutions can be wrong — exactly the "stable for the lifetime of a deploy" assumption the PR's own comment states, except the map outlives the deploy. - Both maps can grow to roughly one entry per class file (the symlink fallback feeds every URL in `info.list` through `isIncludedIn`) and are then retained for the entire life of the running context. Suggested fix: clear both maps in `configureStop()` alongside the existing clear. If you also want the memory back on a normal successful start, clearing on the after-start event would do that too, but `configureStop()` parity is the minimal consistent fix. ### Style, conventions, tests - Code style matches the file (final locals, `// symb links` comments carried over, plain collections). The new explanatory comments are wordier than the surrounding code but justify non-obvious constraints; I'd keep them. - No new imports needed — `HashMap`/`Map` are already imported, so the diff compiles as-is. - **JIRA convention**: TomEE commits reference a `TOMEE-xxxx` issue (e.g. `TOMEE-4657 - …` in recent history). This PR has no JIRA — please file one at https://issues.apache.org/jira/browse/TOMEE and prefix the commit/title. - **Tests**: none added. The class is tightly coupled to Tomcat lifecycle, so a unit test is awkward, and "no behavior change" claims are indirectly covered by the existing webapp deployment ITs. That's acceptable, but a small test around `canonical()`'s IOException fallback and the early return would be cheap if the methods were made package-private for testing. ### Verdict Good, well-motivated optimization with measured results, and the single-threaded logic checks out completely. I'd request two changes before merge: switch the two caches to `ConcurrentHashMap` (parallel annotation scanning makes plain `HashMap` unsafe here), and clear them in `configureStop()` so a context reload doesn't run against stale canonical resolutions. Both are one-line-ish fixes that don't affect the measured win. -- 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]
