jetty fixes II#6392
Conversation
duncdrum
force-pushed
the
dp-fix-jetty-cont
branch
from
d4d5236 to
90e89b4
Compare
joewiz
left a comment
joewiz
left a comment
There was a problem hiding this comment.
[This response was co-authored with Claude Code. -Joe]
Nice work @duncdrum — read through end-to-end, all 10 commits are clean and CI green across the full matrix (ubuntu/macOS/windows integration, ubuntu unit, container, W3C). The disciplined commit structure makes this very pleasant to follow. A few observations worth flagging for future maintainability; none are blockers.
1. WindowsPathResource extends Resource (not PathResource)
The docstring acknowledges that Jetty internals using instanceof PathResource won't recognise wrapped resources, and says CI validates the exploded-webapp path doesn't depend on that. Worth asking — have you also audited eXist's own code for instanceof PathResource (or getClass() == PathResource.class) call sites that might silently get the wrong answer for a wrapped resource? A quick grep should put that to bed if not already done.
2. MimeTable parameterised getInstance overloads
Each variant is correctly double-checked-locked individually, but if two threads call different overloads concurrently during init (e.g. getInstance() and getInstance(Path)), the singleton seed is whichever wins the race — they don't coordinate with each other. In practice the singleton is initialised once at startup so this is latent, but a single shared lock (or making the three overloads delegate to one canonical entry point) would close the door on the footgun. Optional polish, not a blocker.
3. LEGACY_TOKEN_SEPARATOR retention
The dual-separator parsing in PersistentLogin is the right call for backward compat with cookies already in users' browsers. A one-line comment along the lines of "safe to remove once all v7-era persistent-login cookies have expired — target eXist 8.x" would help a future maintainer know when to retire it. (Browser cookies expire; we don't need this forever.)
4. CHAIN_PRIORITY_* constants
The gaps (0 → 100 → 1000 → MAX_VALUE) are reasonable for future insertion room, but a one-line comment explaining the spacing — e.g. "100s reserve room for pre-Lucene workers, 1000+ for Lucene-and-later, MAX for legacy unranked" — would help future contributors pick a value for a new index without guessing.
Otherwise
The substantive engineering choices are all well-motivated:
JettyStart's migration off deprecatedObservableto a typed listener APIIndexControllerswappingHashMap→LinkedHashMap+ explicit comparator (fixes a real latent ordering bug)HttpResponseWrapper's explicit empty-Path handling for standalone Jetty'sgetContextPath()=""ExistWebServer.builder()replacing the boolean-arg constructor chain- The Windows-only IT in
exist-webdav(gated to Failsafe) — exactly the right shape
LGTM modulo the observations above.
|
@duncdrum Let me know if the comments above were useful at all or not. Happy to pass this info along whenever you'd like. |
|
Thanks @joewiz
Also noted a test gap for the portal. So I added |
line-o
left a comment
line-o
left a comment
There was a problem hiding this comment.
It's a lot but it it's also a lot of good stuff in one PR
|
Thx @line-o yes it was quite a journey. The next one is coming up with the http client. The changes here should make our tests less flaky, and the interaction with Jetty more robust. It sets us on the path to have login work properly again. So yay for that. |
Jetty 12.1 resolves sub-paths with path.resolve(uri.getPath()), which throws InvalidPathException for Windows drive URIs (/D:/...). Exploded test webapps fail in WebAppContext.getWebInf() with available=false and 503 on every path. WindowsPathResource wraps PathResource on Windows and restores Jetty 12.0 resolve behaviour via Paths.get(resolvedUri). Enable throwUnavailableOnStartupException so startup failures surface immediately in CI.
HashMap iteration order is undefined; Lucene could run before structural indexing during store/flush. Use explicit ordering: structural, statistics, Lucene, then others. Unrelated to the Jetty 12.1 upgrade; bundled for review convenience.
Block in JettyStart until required contexts reach isAvailable(); fail fast from ExistWebServer with embedded startup detail. Set exist.jetty.portal.dir for distribution-mode tests. Remove LoginModuleIT TCP probe now redundant.
- WindowsPathResourceIT in exist-webdav (Windows CI integration job) - jetty-util test dependency for dependency analyze - org.exist.jetty WARN logging in integration test log4j2 configs - ControllerTest @ClassRule (one server per class)
…tics - Symmetric equals via delegate unwrapping - Reorder wrapIfNeeded guards to prevent double-wrap on Windows - Use OSUtil.isWindows() consistently - Remove System.err from recordStartupFailure; rely on logger + test exception detail - Document instanceof PathResource limitation in WindowsPathResource javadoc
…uilder - IndexWorker.getChainPriority() replaces IndexController substring ranking - JettyStartListener replaces deprecated Observable for Jetty lifecycle signals - Event-driven webapp readiness wait with CountDownLatch - org.exist.jetty.startup.timeout.ms override; EXIST/PORTAL context constants - ExistWebServer.builder() for readable test configuration
….xml Replace silent LOG.error + empty maps with IllegalStateException so a bad or missing mime-types.xml fails fast instead of breaking MIME handling across REST, WebDAV, and XML-RPC.
On root context deployments getContextPath() returns "", which produced Set-Cookie Path="" and caused clients to ignore org.exist.login. Map empty context paths to "/" in login.xql and omit blank paths in HttpResponseWrapper. Use "|" as the token separator so strict Set-Cookie parsers accept the value. LoginModuleIT failed at step 3 (admin→guest) because HttpClient 4.x DEFAULT (NetscapeDraftSpec) rejects Jetty 12 RFC6265 Expires dates. Use CookieSpecs.STANDARD with a shared cookie store instead of manually parsing Set-Cookie headers; apply the same spec in AbstractHttpTest for future ITs. Refs jetty/jetty.project#12771
Extract run() startup phases and WebAppReadinessAwaiter to bring NPath scores under the PMD threshold without changing behaviour.
Add PortalRedirectTest for GET / in distribution layout. Deploy the portal with org.exist.jetty.WebAppContext so Windows PathResource wrapping applies, and skip BrokerPool.stopAll when that static-only context stops. Document CHAIN_PRIORITY spacing, WindowsPathResource instanceof audit, and LEGACY_TOKEN_SEPARATOR removal tied to HC4 Phase 5 in PR eXist-db#6393.
duncdrum
force-pushed
the
dp-fix-jetty-cont
branch
from
6f81d06 to
2239330
Compare
4 participants
Summary
Follow-up fixes from the Jetty 12.1 upgrade work in #6390. Addresses Windows CI failures, flaky integration-test startup, persistent-login cookies under Jetty 12, and several issues found while hardening the test harness.
see jetty/jetty.project#15011
What changed
Jetty 12.1 runtime and Windows
Integration test reliability
Persistent login (Jetty 12 cookies)
Other fixes
References
Test plan