Track the event being dispatched per thread in EventObjectSupplier

[iloveeclipse]

EventObjectSupplier published the event being delivered in a single instance-wide map keyed by topic, and DIEventHandler.handleEvent added it, resolved the requestor's arguments and removed it again around each delivery. EventAdmin.sendEvent() delivers on the caller's thread, so several threads can run that sequence for the same topic concurrently and interfere with each other:

• get() did a check-then-act (containsKey, then get) on that map without holding its monitor. When another thread removed the entry in between, get() returned null although its contract allows only the event or IInjector.NOT_A_VALUE. null counts as resolved, so handlers were invoked with a null event, for example java.lang.NullPointerException: Cannot invoke "org.osgi.service.event.Event.getProperty(String)" because "event" is null at ...perspectiveswitcher.PerspectiveSwitcher.handleLabelEvent On the path where the requested type is not Event, get() dereferenced the removed entry and threw the NPE inside the supplier itself.
• One thread's removal could drop the event another thread had just published, so that handler was silently never invoked.
• One thread's event could overwrite the event another thread was about to resolve, so that handler was invoked with a foreign event.

The event is published, resolved and withdrawn by one and the same thread, so it is now kept in a ThreadLocal and no longer visible to other threads. A stack per topic keeps the outer event current when a handler sends another event of the same topic while being injected, and the removal is done in a finally block so a failing resolution no longer leaves a stale event behind.

Also read the current event once instead of three times, compare the desired class null-safely, since getDesiredClass() may return null for types that are neither a Class nor a ParameterizedType, and return null from getTopic() when the descriptor carries no @EventTopic qualifier instead of throwing.

Fixes #2974 Assisted-by: Copilot with Claude Opus 5

[iloveeclipse]

Since I wasn't involved in DI event handling before, I've asked AI to explain the grand design around the current problem. Here is the analysis:

e4 DI event handling: IEventBroker, EventObjectSupplier and the currentEvents ThreadLocal

Background notes for the fix in EventObjectSupplier, which replaces the shared
Map<String, Event> currentEvents by a thread-confined one.

The cast

Component	File	Responsibility
IEventBroker / EventBroker	eclipse.platform.ui/bundles/org.eclipse.e4.ui.services/src/org/eclipse/e4/ui/services/internal/events/EventBroker.java:53-65	Producer-side convenience API. send() → eventAdmin.sendEvent() (synchronous, delivered on the caller's thread), post() → eventAdmin.postEvent() (asynchronous, delivered on EventAdmin's own thread). Wraps the payload into an Event under the key org.eclipse.e4.data (= EventObjectSupplier.DATA).
UIEventPublisher	eclipse.platform.ui/bundles/org.eclipse.e4.ui.workbench/src/org/eclipse/e4/ui/internal/workbench/UIEventPublisher.java:61	EMF adapter on the application model. Every model change calls eventManager.send(topic, argMap), i.e. synchronously on whatever thread touched the model. This is the main source of UIEvents.* traffic.
OSGi EventAdmin	framework	Routes an Event to every registered EventHandler service whose event.topics match. For sendEvent it does so in the caller's stack.
EventObjectSupplier	eclipse.platform/runtime/bundles/org.eclipse.e4.core.di.extensions.supplier/src/org/eclipse/e4/core/di/internal/extensions/EventObjectSupplier.java	The @EventTopic ExtendedObjectSupplier. Two jobs: (a) on injection, register one EventHandler (DIEventHandler) per {requestor, topic} and remember the ServiceRegistration; (b) when that handler fires, hand the event to the injector.
UIEventObjectSupplier	eclipse.platform.ui/bundles/org.eclipse.e4.ui.di/src/org/eclipse/e4/ui/internal/di/UIEventObjectSupplier.java	The same for @UIEventTopic, except that execute() is pushed onto the UI thread.
Requestor / MethodRequestor	eclipse.platform/runtime/bundles/org.eclipse.e4.core.di/src/org/eclipse/e4/core/internal/di/Requestor.java:158, MethodRequestor.java:40-58	Represents one injected method or field. resolveArguments() → InjectorImpl.resolveArguments(this, ...); the values land in actualArgs. execute() only replays actualArgs – it does not resolve anything.
InjectorImpl	.../org/eclipse/e4/core/internal/di/InjectorImpl.java:516-520	For each parameter, finds the supplier by qualifier and calls extendedSupplier.get(descriptor, requestor, track, group) inline.
ProviderHelper	.../org/eclipse/e4/core/internal/di/osgi/ProviderHelper.java:59-95	Looks the supplier up as an OSGi service once and caches it in a static map – one supplier instance for the whole framework, shared by every context, every requestor and every thread. It also performs injector.inject(supplier, objectSupplier) here, which is how UIEventObjectSupplier.uiSync ever gets a value.

One delivery, end to end

1. Someone changes the model, or calls broker.send(topic, data) → EventAdmin.sendEvent on thread T.
2. EventAdmin calls DIEventHandler.handleEvent(event) – still on T.
3. The handler makes the event "current" (addCurrentEvent), then calls requestor.resolveArguments(false) – still on T.
4. InjectorImpl walks the parameters, sees the @EventTopic qualifier, gets the (singleton) supplier and calls get(...) – still on T. get() looks up the current event for that topic and returns the Event or its DATA payload; the value is stored in requestor.actualArgs.
5. The handler withdraws the event (removeCurrentEvent) and calls requestor.execute(), which invokes the target method with the already resolved actualArgs.

broker.send(topic, data)            thread T
  └─ EventAdmin.sendEvent
       └─ DIEventHandler.handleEvent
            ├─ addCurrentEvent(topic, event)      ── publish
            ├─ requestor.resolveArguments(false)
            │    └─ InjectorImpl.resolveArguments
            │         └─ EventObjectSupplier.get( @EventTopic ) ── read
            ├─ removeCurrentEvent(topic)          ── withdraw
            └─ requestor.execute()   // replays actualArgs
                 (UIEventObjectSupplier: uiSync.syncExec(requestor::execute))

So currentEvents is nothing but a side channel to smuggle one value through the injector, which has no API to pass a value down into a supplier. Publish, read and withdraw all happen inside a single synchronous call stack.

Why ThreadLocal is safe – and necessary

The safety argument is exactly that invariant:

• Same thread. Step 3 → 4 → 5 is one uninterrupted stack on thread T. InjectorImpl never hands off to a job, an executor or a Display; it calls get() directly. So whatever T publishes, T is the one that reads it.
• Nobody else has a legitimate interest in that value. A get() call on another thread that is not dispatching must return NOT_A_VALUE: the argument stays unresolved and the injector skips the handler. Sharing the value across threads was never a feature, it was the bug.
• execute() needs no event. MethodRequestor.execute() uses actualArgs, so UIEventObjectSupplier can legitimately push only that part to the UI thread via syncExec – the thread local is already read (and withdrawn) by then.
• The supplier is a singleton (ProviderHelper's static cache). That is precisely why a plain field was wrong: all deliveries of all topics on all threads shared one map, so concurrent sendEvents – a model change on one thread, a job on another – stomped on each other and produced null, a foreign event, or nothing at all.
• Nesting is the one same-thread complication: a handler may send another event of the same topic while it is being injected. That is what the Deque per topic handles; previously the inner delivery destroyed the outer one.
• try/finally around resolveArguments, plus currentEvents.remove() once the map empties, keep a failed injection from leaving a stale event behind and keep no state on event delivery threads.

The guard rail

This design is only correct as long as a subclass resolves arguments on the delivering thread. UIEventObjectSupplier does – only execute is moved. That assumption is what the Javadoc on currentEvents records: if someone ever wrapped resolveArguments itself in a syncExec or a Job, the value would have to travel with the requestor instead.

[iloveeclipse]

@fipro78 , @vogella : I see your names in the git history: can you review this PR?

[Copilot]

Copilot review overview

🟡 Changes recommended

The bundle version must be incremented and deterministic concurrency regression coverage added.

Review effort: Balanced
Findings: 1 · 1

Open (2)

• Add regression tests for concurrent and nested same-topic delivery · New
• Increment bundle service version for the implementation fix · New

What changed in this PR

Fixes concurrent and re-entrant event injection in EventObjectSupplier.

Changes:

• Tracks active events per thread and topic using stacks.
• Ensures cleanup after argument-resolution failures.
• Adds null-safe event and qualifier handling.

File	Description
EventObjectSupplier.java	Isolates event dispatch state and improves cleanup and null handling.

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[eclipse-platform-bot]

This pull request changes some projects for the first time in this development cycle.
Therefore the following files need a version increment:

runtime/bundles/org.eclipse.e4.core.di.extensions.supplier/META-INF/MANIFEST.MF

An additional commit containing all the necessary changes was pushed to the top of this PR's branch. To obtain these changes (for example if you want to push more changes) either fetch from your fork or apply the git patch.

Git patch

From 230fccfe8bab0555de0d272cfa43b9c286ccc113 Mon Sep 17 00:00:00 2001
From: Eclipse Platform Bot <platform-bot@eclipse.org>
Date: Mon, 28 Sep 2026 13:18:29 +0000
Subject: [PATCH] Version bump(s) for 4.42 stream


diff --git a/runtime/bundles/org.eclipse.e4.core.di.extensions.supplier/META-INF/MANIFEST.MF b/runtime/bundles/org.eclipse.e4.core.di.extensions.supplier/META-INF/MANIFEST.MF
index cd948cf00a..b6affcbaf6 100644
--- a/runtime/bundles/org.eclipse.e4.core.di.extensions.supplier/META-INF/MANIFEST.MF
+++ b/runtime/bundles/org.eclipse.e4.core.di.extensions.supplier/META-INF/MANIFEST.MF
@@ -3,7 +3,7 @@ Bundle-ManifestVersion: 2
 Bundle-Name: %Bundle-Name
 Bundle-Vendor: %Bundle-Vendor
 Bundle-SymbolicName: org.eclipse.e4.core.di.extensions.supplier
-Bundle-Version: 0.17.1200.qualifier
+Bundle-Version: 0.17.1300.qualifier
 Bundle-RequiredExecutionEnvironment: JavaSE-17
 Require-Capability: osgi.extender;
   filter:="(&(osgi.extender=osgi.component)(version>=1.3)(!(version>=2.0)))"
-- 
2.55.0

Further information are available in Common Build Issues - Missing version increments.

[github-actions]

Test Results

    54 files  ± 0      54 suites  ±0   58m 19s ⏱️ - 1m 54s
 4 844 tests + 8   4 822 ✅ + 8   22 💤 ±0  0 ❌ ±0 
12 423 runs  +24  12 269 ✅ +24  154 💤 ±0  0 ❌ ±0 

Results for commit 5bdebfc. ± Comparison against base commit dacbef1.

♻️ This comment has been updated with latest results.

[Copilot]

Copilot review overview

🟢 Approval recommended

The implementation addresses the reported races and includes reliable regression coverage for the principal failure modes.

Review effort: Balanced
Findings: None

Resolved since last review (2)

• Add regression tests for concurrent and nested same-topic delivery
• Increment bundle service version for the implementation fix

[iloveeclipse]

@fipro78 , @vogella : I see your names in the git history: can you review this PR?

I plan to merge today, would be early enough for M1. If you have any objections, speak up.

[vogella]

Please ask your AI agents to generate concise Javadoc and update the PR. We do not want stuff like this in our code:

/**
 * Verifies that {@link EventObjectSupplier} hands an event to the injector if and only if the
 * calling thread is the one currently delivering it.
 * <p>
 * The supplier is a singleton OSGi service shared by all contexts and all threads, and it has no
 * way to pass the event down into the injector other than parking it for the duration of
 * {@link IRequestor#resolveArguments(boolean)}. Because {@code EventAdmin.sendEvent(Event)}
 * delivers on the caller's thread, several threads can be inside that window at the same time, and
 * a handler may even re-enter it on the same thread while its arguments are being computed.
 * </p>
 * <p>
 * The deliveries below are driven through the handler the supplier creates for a requestor, so that
 * publishing, resolving and withdrawing happen in the same order as in production. The
 * interleavings are forced with latches instead of with concurrent load, so a regression fails
 * reliably rather than occasionally.
 * </p>
 */

[vogella]

UIEventObjectSupplier still has the old pattern. Please also update it.

[iloveeclipse]

UIEventObjectSupplier still has the old pattern. Please also update it.

Old pattern of what exactly? UIEventObjectSupplier extends EventObjectSupplier, and EventObjectSupplier.currentEvents is a private field not exposed to subclasses.

[iloveeclipse]

Please ask your AI agents to generate concise Javadoc and update the PR. We do not want stuff like this in our code:

Can you point what exactly is not concise in the referenced Javadoc? I would it explains pretty good what the test is about, especially there is zero documentation on whole DI project.

[vogella]

/**

• Verifies that {@link EventObjectSupplier} hands an event to the injector if and only if the
• calling thread is the one currently delivering it.
• The supplier is a singleton OSGi service shared by all contexts and all threads, and it has no
• way to pass the event down into the injector other than parking it for the duration of
• {@link IRequestor#resolveArguments(boolean)}. Because {@code EventAdmin.sendEvent(Event)}
• delivers on the caller's thread, several threads can be inside that window at the same time, and
• a handler may even re-enter it on the same thread while its arguments are being computed.
• The deliveries below are driven through the handler the supplier creates for a requestor, so that
• publishing, resolving and withdrawing happen in the same order as in production. The
• interleavings are forced with latches instead of with concurrent load, so a regression fails
• reliably rather than occasionally.

*/

If you ask your AI engine to make it concise you get for example:

/**
 * Verifies that {@link EventObjectSupplier} hands an event to the injector only on the thread
 * currently delivering it, including under concurrent and re-entrant delivery.
 * <p>
 * Interleavings are forced with latches rather than concurrent load, so a regression fails
 * reliably.
 * </p>
 */

Adding a lot fo Javadoc (AI Slop) only leads to people not reading it.

[vogella]

UIEventObjectSupplier still has the old pattern. Please also update it.

Old pattern of what exactly? UIEventObjectSupplier extends EventObjectSupplier, and EventObjectSupplier.currentEvents is a private field not exposed to subclasses.

True, forget this feedback.

[iloveeclipse]

Adding a lot fo Javadoc (AI Slop) only leads to people not reading it.

I'm with you if it would be a typical AI generated "getter like" javadoc explaining three times what some "get" method is doing.

In this particular case test class javadoc helps reader to understand complex system context which is not documented anywhere, so I assume it can also help others. The first part of the comment explains the related design details and the rest of the comment explains how the test addresses the complex behavior of the system.

The proposed shorter version is not providing enough details to understand the problem and the added test.

[vogella]

I will not block this PR due to over long AI generated Javadoc, but please get another opinion from another committer before committing this.

@iloveeclipse if you have the option in your tooling switch for future contributions to Opus 5.5 or the latest Astra. Opus 5.0 is "famous" for generating AI slot in comments. Opus 5.5 is also much cheaper in terms of token usage and better in code quality.

[iloveeclipse]

get another opinion from another committer before committing this.

@eclipse-platform/eclipse-platform-committers : I've asked Dirk for a review (he seem to be involved in DI before), but he is not answering. Any candidates for a review here?

Problem:
EventObjectSupplier stored the event currently being delivered in a shared instance-wide map, so concurrent deliveries of the same topic on different threads could drop, overwrite, or null out each other's event (causing NPEs and missed/foreign events in handlers).

Fix:
The event is now held in a ThreadLocal stack per topic (removed in a finally block), so publishing, resolving and withdrawing stay confined to the delivering thread and nested same-topic sends still restore the outer event.

[merks]

For what it's worth, I think for user-facing API documentation should most definitely take into consideration being concise and avoiding redundancy. For developer/contributor/committer facing documentation, especially in a test, a little bit more verbiage to explain what the test exactly is doing seems helpful and more acceptable. Details about why a thread local is being used on the private variable also seem useful.

Overall the changes look sound and I trust @iloveeclipse's good judgement that they are sound.

[fipro78]

Sorry for the late reply. I looked into the changes, but I really can't say anything about the changes in detail. It sounds reasonable, the multithreading seems to be addressed before, but looks like not good enough. And to be honest, I was more familiar with the whole topic a while ago.

Since you added tests and nothing seems to be broken with your changes, I believe it is fine.

[iloveeclipse]

Sorry for the late reply.

No need to sorry and thanks for the reply, I'm busy as well and hardly can answer on all mails flying around.

I will merge now, should there be any regressions, we should find & fix them early enough.