Ticket #1934 (closed defect: fixed)

Opened 6 years ago

Last modified 6 years ago

Potential unwanted dependency between ModuleService and MenuService

Reported by: MichaelZinsmaier Owned by: curtis
Priority: major Milestone: imagej2-b8-analysis
Component: SciJava Common Version:
Severity: serious Keywords:
Cc: Blocked By:
Blocking:

Description

The KNIME ImageJ2 integration produces a null pointer exception if the ModuleService comes before the MenuService.

With the beta 7 release of ImageJ2 we had to move ModuleService behind MenuService in the constructor call of the scijava ImageJ Context to avoid a null pointer during initialization (onEvent(ModulesAddedEvent) was called before initialize).

@Curtis

What's weird to me is, I am not sure why we don't see this bug when running the end-user ImageJ application...

Maybe because we use the Context(ServiceClass[]) constructor?

Thanks in advance
Michael

Change History

comment:1 Changed 6 years ago by curtis

I was able to reproduce this bug by changing imagej.Main to:

ImageJ ij = new ImageJ(ModuleService.class, MenuService.class);

And the same NPE can be witnessed.

This bug is a symptom of a race condition, introduced by some changes to how services manage their event handling. It used to be that each service would subscribe itself to events when finished initializing. But that logic was generalized, such that all "contextual" objects (including services) now subscribe themselves as soon as their context is assigned. So now, MenuService subscribes itself to e.g. ModuleAddedEvents before it is finished initializing.

I believe I have fixed the problem in scijava-common: services now defer event handler registration until the end of initialization. If a service overrides the initialize() method, it is important for it to call super.initialize(), or else its event handlers will not be registered. Relevant changeset is:  b019a06a.

I will cut a new release of scijava-common soon and update ImageJ2 to use it; will close this bug when that is done, tested and working.

Last edited 6 years ago by curtis (previous) (diff)

comment:2 Changed 6 years ago by curtis

  • Cc CurtisRueden removed

comment:3 Changed 6 years ago by curtis

  • Status changed from new to closed
  • Resolution set to fixed

It really bothered me that calling super.initialize() was now required for any services overriding initialize(). I think that requiring such a thing is very suboptimal and would result in many future programming errors. So I reworked the fix slightly to avoid that need:  2f08d695.

The change does introduce another Service API method, which breaks downstream Service implementations (though anything extending AbstractService is in the clear). But I think the pain is worth it to fix this critical bug.

I've released SciJava Common 1.5.0 which includes this fix, and updated ImageJ2's master branch to use it:  a229f9ad.

comment:4 Changed 6 years ago by curtis

  • Milestone set to imagej2-b8-analysis
Note: See TracTickets for help on using tickets.