This Bugzilla instance is a read-only archive of historic NetBeans bug reports. To report a bug in NetBeans please follow the project's instructions for reporting issues.

Bug 169844 - MetaInfServicesLookup delivers change events inside Lookup.Result.allInstances
Summary: MetaInfServicesLookup delivers change events inside Lookup.Result.allInstances
Status: RESOLVED FIXED
Alias: None
Product: platform
Classification: Unclassified
Component: Lookup (show other bugs)
Version: 6.x
Hardware: All All
: P3 blocker (vote)
Assignee: Jaroslav Tulach
URL:
Keywords: THREAD
Depends on:
Blocks: 159714
  Show dependency tree
 
Reported: 2009-08-05 00:01 UTC by Jesse Glick
Modified: 2009-08-08 07:03 UTC (History)
1 user (show)

See Also:
Issue Type: DEFECT
Exception Reporter:


Attachments
Suggested patch (4.90 KB, patch)
2009-08-05 00:05 UTC, Jesse Glick
Details | Diff
Non-listening portion of test restored (4.92 KB, patch)
2009-08-05 20:38 UTC, Jesse Glick
Details | Diff
Revised patch; events delivered asynch, so ProxyLookup becomes consistent eventually (4.76 KB, patch)
2009-08-07 02:14 UTC, Jesse Glick
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Jesse Glick 2009-08-05 00:01:13 UTC
From http://www.netbeans.org/nonav/issues/showattachment.cgi/77779/deadlock.txt in issue #159714:
...
	at org.netbeans.modules.editor.settings.storage.StorageImpl$Filters$1.resultChanged(StorageImpl.java:494)
	at sun.reflect.GeneratedMethodAccessor32.invoke(Unknown Source)
	at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:25)
	at java.lang.reflect.Method.invoke(Method.java:597)
	at org.openide.util.WeakListenerImpl$ProxyListener.invoke(WeakListenerImpl.java:450)
	at $Proxy2.resultChanged(Unknown Source)
	at org.openide.util.lookup.AbstractLookup.notifyListeners(AbstractLookup.java:518)
	at org.openide.util.lookup.MetaInfServicesLookup.beforeLookup(MetaInfServicesLookup.java:123)
	at org.openide.util.lookup.AbstractLookup$R.beforeLookup(AbstractLookup.java:1095)
	at org.openide.util.lookup.ProxyLookup$R.myBeforeLookup(ProxyLookup.java:653)
	at org.openide.util.lookup.ProxyLookup$R.computeResult(ProxyLookup.java:518)
	at org.openide.util.lookup.ProxyLookup$R.allInstances(ProxyLookup.java:489)
	at org.netbeans.modules.editor.settings.storage.StorageImpl$Filters.rebuild(StorageImpl.java:505)
	at org.netbeans.modules.editor.settings.storage.StorageImpl$Filters.access$400(StorageImpl.java:456)
	at org.netbeans.modules.editor.settings.storage.StorageImpl$Filters$1.resultChanged(StorageImpl.java:491)
...

As pointed out by vstejskal, it is indeed surprising that calling Lookup.Result.allInstances, which sounds like a
"read-only" method, can in fact trigger changes to be delivered to listeners. This is because
MetaInfServicesLookup.beforeLookup checks whether an interface (call it I.class) is being queried for the first time,
and if so, computes the result and notifies it.

Perhaps the notification should simply be deleted. There is no "change" visible to someone watching a Lookup.Result<I>,
because they would already have triggered search of META-INF/services/I before.

The only use case for this notification that I can think of is if I1 extends I2, an impl is registered using
@ServiceProvider(service=I1.class), someone is listening to Lookup.Result<I2>, and someone else asks for
Lookup.Result<I1>.allInstances() - then the listener on I2 gets notified that some instances of I2 appeared out of
nowhere. But expecting MISL to provide subtypes is a mistake to begin with, since in general you will not get instances
registered under a different interface - you can only get lucky. So not getting notifications if some such serendipitous
instances appear does not seem like a significant bug to me.
Comment 1 Jesse Glick 2009-08-05 00:05:06 UTC
Created attachment 85791 [details]
Suggested patch
Comment 2 Jaroslav Tulach 2009-08-05 08:23:10 UTC
Yes, MISLookup delivers change events inside allInstances. There has already been few bugs closed as wontfix regarding 
this behaviour.

Jesse, you cannot delete the whole test. You can only delete its listening part. At the end there still need to be two 
instances visible for anyone who queries the superinterface. Btw. this needs to be guaranteed even if the result is 
wrapped by few ProxyLookup which cache their results.

Unless this is addressed I cannot accept your patch.
Comment 3 Jesse Glick 2009-08-05 20:38:30 UTC
Created attachment 85871 [details]
Non-listening portion of test restored
Comment 4 Jesse Glick 2009-08-05 20:42:23 UTC
There are still two instances for anyone looking for the supertype. Of course this will not working through a caching
proxy lookup, but that is just too bad - you shouldn't be looking for the supertype to begin with - and anyway not
fixable when we use the observer pattern to implement proxying.
Comment 5 Jaroslav Tulach 2009-08-06 09:29:54 UTC
Until the results via ProxyLookup are consistent, I am not going to accept any change of this kind. 

I agree firing events from getter is wrong. I can live with delivering no events or events in another dispatch thread. 
But results have to stay consistent. Btw. why don't you pass RP.getDefault() or no-op Executor to 
MetaInfServicesLookup super class constructor? Imho that shall work and even keep consistency of cached ProxyLookup 
(shall to be tested).

Y01 Don't use WeakSet - Lookup classes are independent on the rest of openide.util. Verified by 
http://hudson.apidesign.org/job/lookup/
Comment 6 Jesse Glick 2009-08-07 02:13:26 UTC
To Y01 - if we wish to keep this separation then we should create org.netbeans.modules.lookup (with module.jar.dir=lib)
and make org.openide.util depend on it.


Attaching patch which delivers events asynch, which is ugly but probably better than the current state. (I did not find
any super constructor taking Executor, so I'm not sure what you mean by that.)

Ideally no events would be delivered at all unless there was actually someone listening on a strict supertype of the
queried interface. Not sure how that would be implemented. Or maybe this already works by the nature of lookup listeners.

A previous iteration of the code + test failed for me unexpectedly when the NotifyListeners _constructor_ delivered
events to listeners (via AL.R.collectFires), but I cannot reproduce with the current version. You are probably the only
one who understands this code so I will not delve into it.
Comment 7 Jesse Glick 2009-08-07 02:14:24 UTC
Created attachment 85933 [details]
Revised patch; events delivered asynch, so ProxyLookup becomes consistent eventually
Comment 8 Jaroslav Tulach 2009-08-07 15:43:03 UTC
Thanks: core-main#00f02ba81b1c

Re. org-openide-util-lookup.jar - yes, I can create it. Not sure if for 6.8, but report an issue if you feel it is 
important.
Comment 9 Quality Engineering 2009-08-08 07:03:17 UTC
Integrated into 'main-golden', will be available in build *200908080201* on http://bits.netbeans.org/dev/nightly/ (upload may still be in progress)
Changeset: http://hg.netbeans.org/main-golden/rev/00f02ba81b1c
User: Jaroslav Tulach <jtulach@netbeans.org>
Log: #169844: Accepting Jesse's patch: Listeners shall not be called from getter like methods