scijava
DefaultEventBus#publishLater should use queue instead of run
@dscho and @ctrueden -
Is it possible that having publishLater call EventService#run instead of EventService.queue could be violating some assumptions in IJ2?
At the least we should probably document the various publish methods in DefaultEventBus, as I'm not sure the expected behavior of a publishLater method would be to run it immediately on a new thread - especially given that publishNow uses ThreadService#invoke which appears to only run threads on the EDT... so the behavior of these two methods seems reversed/counter-intuitive without documentation.
Yes, as we discussed @hinerm, I think 6b878da41160da695f6552ee86f537cff2443faf was an error because the contract of publish, publishNow and publishLater is for them to always execute things on the EDT (for some definition of "EDT" :wink:). It may be orders of magnitude faster, but it also violates that contract, meaning that non-EDT threads now receive event notifications which has really bad ramifications.
And I agree about adding javadoc for the DefaultEventBus API. (Tangentially: we should rename DefaultEventBus to SciJavaEventBus, since there is no corresponding org.scijava.event.EventBus interface.)
Whatever we do, we must not have the performance regression that adds a couple of seconds to the Context creation just because events are published synchronously...
@dscho: It would not be a "regression" because the "fix" that "improved performance" actually broke behavior. By the same logic, I could simply change the Context() constructor to call this(true) instead of this(false), claim an orders-of-magnitude "performance increase" and then call it a "performance regression" when someone inevitably reverts that broken change.
However, that said, of course I agree wholeheartedly that we should do whatever we can to avoid such high overhead when publishing events. But not at the cost of correctness—the event subsystem is too critical for that.
A compromise, then. Start a thread that adds a thread to the EDT. So that the 100ms do not punish the current thread.
@dscho: Sure, if doing that is truly faster, then I'm all for it. It's bizarre to me that calling EventQueue.invokeLater directly pays such a heavy price. :sweat:
Years later, this rears its head again. When debugging some status-reporting issue with @etadobson, we noticed that dozens of new threads were spawned—in fact one per call to statusService.updateStatus(...), because it publishes a StatusEvent which in turn calls EventService#publishLater which (contrary to documentation) then calls ThreadService#run rather than ThreadService#queue. I'm going to escalate this to a bug finally, and will try to find time to implement Johannes's suggestion above to spawn a thread that queues the event to the EDT, and then test how it impacts both behavior and performance.