mattcasters opened a new pull request, #8108: URL: https://github.com/apache/hop/pull/8108
Fixes #8047 Hop Web creates one RAP `UISession` (and one SWT `Display`) per browser session. Several JVM-wide statics still held widgets, images, and session GUI identity. Concurrent users then hit `Invalid thread access` and disposed `UISession`/widget errors. This isolates that GUI state. It is **session isolation**, not full multi-tenancy: `hop-config.json`, projects/environments, and `HopMetadataInstance` stay process-wide on purpose. ## What changed Three isolation patterns, matching what `HopGui` / `GuiResource` already do: 1. **ui-module classes** (`HopGuiKeyHandler`, `PropsUi`): `ISingletonProvider` + `rcp/*Impl` (desktop singleton) + `rap/*Impl` (`SingletonUtil.getSessionInstance`). 2. **Plugins** (`GitGuiPlugin`, `GitResource`): `HopGui.getSessionSingleton(...)` because plugin classloaders cannot load RAP `*Impl` via `ImplementationLoader`. 3. **Perspectives**: `getInstance()` prefers `HopGui.findSessionPerspective(...)`; the static `instance` field remains only as a fallback for tests and disabled perspectives. Also: - `SashFormMemory` tracks sash forms per `Display`; `resetAll()` only touches the current session. - `SwtGc` SVG images are cached per `Device`, with a dispose hook on **that** device. - Drill-down engines/buffers are keyed by `hopGui.id` stamped at start (engine threads have no RAP session). Session destroy clears that id. - `HopNamespace` and `TreeMemory` are keyed by `Display`. - Hop Web dark mode is an in-memory `PropsUi` override and does not rewrite `hop-config` for other users. Developer notes: `docs/hop-dev-manual/modules/ROOT/pages/hopweb/hopweb-antipatterns.adoc`. ## Reviewer guide Please review as **crash isolation first**, then session identity. Desktop Hop must behave as today (one `HopGui`, one `Display`). ### 1. Highest crash risk (read these first) | Area | File | What to verify | |---|---|---| | Key handler | `ui/.../HopGuiKeyHandler.java` + `rcp`/`rap` `HopGuiKeyHandlerImpl` | One handler per RAP session. `keyPressed` must not walk `Control`s from another `Display`. Do **not** call `HopGui.getInstance()` from `HopGuiKeyHandler.getInstance()` (breaks `HopGuiKeyHandlerTest`). | | Sash weights | `ui/.../shared/SashFormMemory.java` | `TRACKED_BY_DISPLAY`; `resetAll()` uses `Display.getCurrent()` only; dispose drops that display's map. | | SVG cache | `ui/.../shared/SwtGc.java` | Per-`Device` cache; dispose hook is **per device**, not one process-wide flag. | | Git colors/images | `plugins/misc/git/.../GitResource.java` | No static `Image`/`Color` singleton from the first session's `GuiResource`. | ### 2. Session identity | Area | File | What to verify | |---|---|---| | Perspectives | `Explorer` / `Metadata` / `Execution` / `Configuration` / `Git` / `GitCommit` `getInstance()` | Production lookup goes through `HopGui`. Static `instance` is fallback only (`ExecutionPerspectiveDisabledTest` still works). | | Git plugin | `GitGuiPlugin.java` | `git` (`UIGit`) is an instance field. `getInstance()` uses `HopGui.getSessionSingleton`. Listeners register on **this** session's explorer. | | File dialog | `HopVfsFileDialog` | Open dialog is stored on `HopGui`, not a process-wide static. | | Drill-down | `DrillDownGuiPlugin` + pipeline/workflow graph start | `HOP_GUI_ID` stamped on the engine in the GUI thread. `cleanupOnRunStart(id)` must not clear another session. Do not call `HopGui.getInstance()` from `PipelineStartThreads`. | | Session end | `rap/.../HopWebEntryPoint.java` | `beforeDestroy` still writes audit, then `DrillDownGuiPlugin.cleanupSession(hopGui.getId())`. | ### 3. Desktop / RAP split - `rcp/*Impl`: process-wide static (desktop and most unit tests). - `rap/*Impl`: `SingletonUtil.getSessionInstance`. - `ImplementationLoader` fallback in `HopGuiKeyHandler` / `PropsUi` exists because `hop-ui` unit tests have no `rcp`/`rap` jar. In a real desktop/web runtime the `*Impl` must be used. - Plugin classes must **not** depend on RAP `SingletonUtil`. ### 4. Out of scope (do not expand this PR) These can mix **data** between users but are not the SWT crashes in #8047: - Shared `hop-config.json` / project and environment enablement - Process-wide `HopMetadataInstance` (still set from `HopGui` for VFS callers) - `HopLogStore` / `DefaultLogLevel` - Hop Server execution identity ### 5. Tests already in the PR - `HopGuiKeyHandlerTest` (including two handlers do not share parents) - `DrillDownGuiPluginTest` (cleanup of session A leaves session B) - `GitGuiPluginSessionIsolationTest` (`git` is not static) - `StaticSwtSessionResourceTest` (no new static `Image`/`Color`/`Control`/`Shell`/`SashForm`/`GC` fields under `org.apache.hop.ui`) - `ExecutionPerspectiveDisabledTest` Please run: ``` ./mvnw -pl ui -Dtest=HopGuiKeyHandlerTest,DrillDownGuiPluginTest,StaticSwtSessionResourceTest,ExecutionPerspectiveDisabledTest test ./mvnw -pl plugins/misc/git test ``` Desktop smoke: open Hop GUI, shortcuts, sash drag, run a pipeline, git explorer colors. Hop Web (best check for this bug): two logins from `docker/local-auth-config` on `/ui` and `/ui-dark`. Open explorer/git, drag a sash, run a pipeline, log one user out. The other session must not throw `Invalid thread access` or `Widget is disposed`. ------------------------ Thank you for your contribution! Follow this checklist to help us incorporate your contribution quickly and easily: - [x] Run `mvn clean install apache-rat:check` to make sure basic checks pass. A more thorough check will be performed on your pull request automatically. - [x] If you have a group of commits related to the same change, please squash your commits into one and force push your branch using `git rebase -i`. - [x] Mention the appropriate issue in your description (for example: `addresses #123`), if applicable. To make clear that you license your contribution under the [Apache License Version 2.0, January 2004](http://www.apache.org/licenses/LICENSE-2.0) you have to acknowledge this by using the following check-box. - [x] I hereby declare this contribution to be licensed under the [Apache License Version 2.0, January 2004](http://www.apache.org/licenses/LICENSE-2.0) - [ ] In any other case, please file an [Apache Individual Contributor License Agreement](https://www.apache.org/licenses/icla.pdf). -- 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]
