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.
Attached simple NetBeans Platform Application which reproduces the problem, environment: Product Version: TestProject 200801291616 Java: 1.6.0_04; Java HotSpot(TM) Client VM 10.0-b18 System: Windows XP version 5.1 running on x86; Cp1250; cs_CZ (testproject2) I've created new NetBeans Platform Application, added TestAction (extends AbstractAction) and registered it in layer.xml as Actions/Other/testmodule-TestAction.instance and Menu/File/testmodule-TestAction.shadow (pointing to original testmodule-TestAction.instance). After the application started, two instances of TestAction were created but I expected that only one instance is created based on the structure of layer.xml. Setting priority to P2 as this caused serious problems in real application which were quite hard to debug and time-consuming to workaround (== rewriting tens of such actions to singletons). Real action registered itself as a listener to some component of my application and because of two action instances being registered each event produced duplicate results.
Created attachment 59627 [details] Simple NetBeans Platform Application reproducing the problem
Just a note - for debug purposes '>>> Created TestAction instance #{X}' is sent to the System.err when instance of the action is created, after the application starts you can find the following in logfile: >>> Created TestAction instance #1 >>> Created TestAction instance #2
I guess you do not expect me to fix and integrate this into 6.1, don't you? Otherwise you would have reported this issue a week ago...
No I don't, take your time to investigate:o) I have to use 6.0.1 platform for now, anyway.
Dafe, please take this one.
I think I found it in InstanceDataObject - when instance creation is defined in xml layer through instanceCreate attribute (method call) and there is no instanceClass attribute, infrastructure has no way of finding out the instance class then to create instance and getClass() from it. However, this created instance is forgotten and creation is repeated again when instance is needed. This particular example will be fixable I think, but generally if instanceClass attribute is missing, infrastructure can't guarantee only one instance - when InstanceDataObject.findFO is called, then we can choose - either FO will not be found or we have to instantiate instance definition which is without instanceClass attribute.
fixed with test, unfortunately writing the test took me almost two days, as I'm unfamiliar with the IDO and MenuBar code: http://hg.netbeans.org/core-main/rev/80eea651d4d9 http://hg.netbeans.org/main/rev/80eea651d4d9 Btw: jis, I think using "instanceCreate" methodvalue without "instanceClass" supplied may not be good for performance, instance is created sooner that it could be.
Reopening, I have to rollback the fix, it caused P1 - 141682
This issue is not fixable - system can't guarantee only one instance of registrations which use "instanceCreate" attribute but are missing "instanceClass" attribute. System has no other chance but to actually create instance in order to get class name in this situation. Unfortunately, such instance is created too soon, for example during FolderLookup creation on some registration folder, and so it doesn't work OK, as whole system expects that instance is really created when instanceCreate(..) is called.
Conclusion: Add instanceClass when using instanceCreate attribute. Otherwise your object can be created multiple times (2 times mostly it seems). I added warning log for such situations: http://hg.netbeans.org/main/rev/d20eab2d47d8
Integrated into 'main-golden', available in build *200808010201* on http://bits.netbeans.org/dev/nightly/ Changeset: http://hg.netbeans.org/main/rev/b5daad390520 User: Dafe Simonek <dsimonek@netbeans.org> Log: backout of 80eea651d4d9 (fix of #131951)
I'll report all the warnings to appropriate components
[reference issue #142550] This doesn't look right to me. Please do not file issues for modules to add this attribute until the situation has been properly reviewed. I would recommend #d20eab2d47d8 also be reverted for now. <file name="testmodule-TestAction.instance"> <attr name="instanceCreate" methodvalue="testmodule.TestAction.create"/> </file> is a perfectly good instance registration. Adding an instanceClass attribute should not be necessary, and we have repeatedly told people in various forums not to add it. In general, <attr name="instanceClass" stringvalue="C"/> is just a shortcut for <!-- if needed --><attr name="instanceOf" stringvalue="C"/> <attr name="instanceCreate" newvalue="C"/> There is no guarantee in general that a FileObject attribute will be instantiated only once. The filesystem should cache the created value so long as it is held in some other way (i.e. weakly cache), and FolderInstance may cache instances associated with files for a while, but that is all. If a module breaks due to assuming that an object-valued attribute is created only once, the fault is generally in the module. Anyway no comment in this issue explains to my satisfaction what the real problem here was. No one should be calling InstanceCookie methods on an item in Actions/ or Menu/ except MenuFolder, which should ask for IC.instanceCreate anyway. If this is not the case, I want to see a stack trace.
The original use case was for VisualVM which defines its own folder in layer for contents of its context menus. InstanceCookie is used here to resolve action instances. The problem is that VisualVM defines an action which needs to register itself as a listener to another class. This action needs to have exactly one instance, thus .instance is used in one folder and .shadow at other places of the layer. Expected behavior - only one action instance is created (otherwise the whole .shadow think doesn't make much sense for me). Actual behavior - two instances of the action are created, causing ugly side effects (2 listeners being registered) which are very difficult to debug. I haven't found any explicit note that more than one instance may be created when using .shadow, so if not fixable, I think that current logging the warning is the best approach. I would also strongly suggest to describe this behavior visibly in some docs, showing concrete code examples ensuring that exactly one instance being created (singleton). Given how much other actions could potentially be also affected (based on warnings logging during IDE startup) I'm also cc'ing performance as this may noticeable impact startup performance.
I don't believe the issue has to do with use of *.shadow files, which just delegate their InstanceCookie to the original *.instance file anyway. I certainly agree it is desirable to not create more than one instance from a *.instance. I do not agree that the system is required to do so in all situations - just make a best effort. If registering >1 listener from your Action instance is a real problem rather than just an oddity when debugging (and you have do not use WeakListener, so both listeners remain active indefinitely), then you should register a single heavyweight listener from static code, and have the Action instances register lightweight proxy listeners to this static source (or use WeakSet to enable the static listener to refire changes to all extant instances). What I am opposed to specifically is the use of instanceClass as a workaround for some underlying bug which I have not seen adequately described or investigated. (Why is the object being instantiated more than once? Is the first instance being held strongly by infrastructure code or not? Who was calling InstanceCookie.instanceClass and why?) Probably instanceClass should be deprecated altogether as its functionality is completely subsumed by instanceCreate. (Plus instanceOf where this is necessary, which it should not be for action registrations - generally instanceOf should only be needed for instances in the effectively deprecated Services/ folder.) Furthermore, instanceClass and instanceCreate should be mutually exclusive. Since there has been no comment from Dafe so far, I am opening a separate issue #142836 requesting that the console warning be removed, and that no one be asked to use instanceClass gratuitously. This issue - that sometimes extra instances are created which ideally would not be - can be left as is or reopened independently.
Jesse, I think we have misunderstanding here. Please read my detailed comments above, while I'll try to describe what we have: 1) Bug impact Looking at log info in related 142550, we can see that: - at least 25 menu actions is instantiated twice during every startup - at least 3 navigator panels are instantiated twice during every startup - about 10 other actions in Loaders/text and Editors/text are instantiated twice during every startup - about 12 option panel controllers are instantiated twice when Tools/Options is invoked for first time ...and probably lots of other things which wasn't caught I'll try to attach thread dumps of multiple instantiations 2) State of fix I did what i could and the best thing with which I could came is logging warning and using instanceClass attribute. if anybody come with better solution, I'm perfectly happy with it and I'll remove warning immediately. But so far, I think warning is the best what we have. Certainly we shouldn't let the things as they are now - I expected this issue to be P0 release stopper for performance team.
Of course we should not gratuitously reinstantiate instances. The problem is the workaround with instanceClass, which makes no sense from an API perspective. If I had <file name="x.instance"> <attr name="instanceCreate" methodvalue="some.Factory.create"/> </file> where package some; public class Factory { public static Action create() { return new Action() {...}; } } should I then change my layer to the following? <file name="x.instance"> <attr name="instanceCreate" methodvalue="some.Factory.create"/> <attr name="instanceClass" stringvalue="some.Factory$1"/> </file> This is clearly not an acceptable long-term workaround. I have read your earlier comments in this issue but they do not explain what is needed to look for a proper fix: 1. Exactly what happens in the filesystems/datasystems/settings infrastructure (which module? which line of code?) that causes an object to be instantiated twice. 2. Why adding an instanceClass attribute causes this bug not to appear. Something like InstanceDataObjectModule38420Test should also be useful in debugging the problem, though this test does use instanceClass.
Created attachment 66707 [details] thread dumps of instantiations, which explains what is going on
After off line discussion with jtulach, I'm passing back to him for better fix. Sorry I didn't come with something better myself...
Note I was not exact with my bug impact estimations - most of our actions somehow survive and are instantiated just once. It seems that only actions that extends javax.swing.Action purely are affected.
SystemAction's, or indeed any objects assignable to SharedClassObject, would not be affected. FolderList seems to be calling instanceOf for a reasonable purpose - to find nested Lookup's, before actually creating normal instances. ActionsList is calling instanceOf just to check for JSeparator's, which it will not create. Should BinaryFS$AttrImpl.getValue be caching the value, at least during startup? Seems in general that caching of methodvalue/newvalue/serialvalue attributes is desirable. InstanceDataObject$Ser.instanceCreate does keep a soft cache of the created object, which is fine. There is an obvious problem that getClassName does not use this cache even when it exists! (If a full GC happened to intervene, and there was no extant hard ref, then the object would be recreated - which is OK.)
Right, also see my first fix, http://hg.netbeans.org/main/rev/80eea651d4d9, I tried to fill InstanceDataObject$Ser soft ref cache in getClassName, but the problem is that instance is created "too soon" and it leads to 141682, and possibly other similar issues, which I didn't know how to fix.
I can add a special API to XMLFS and BinaryFS to provide special handling of "instanceClass" for newvalue and methodvalue. In case of newvalue, it is quite easy. In case of methodvalue, I would use the return type of the method - not absolutely correct, but hopefully correct enough.
#80eea651d4d9 looks right to me. What was the problem? "Too soon" for whom exactly?
"Too soon" for org.netbeans.modules.options.colors.FontAndColorsPanelController:82, description of what happened is in 141682. In short, Lookup.forPath("some valid path").lookupApp(Object.class) returned empty result for FontAndColorsPanelController instance - I wasn't able to exactly find out why.
"Lookup.forPath("some valid path").lookupAll(Object.class) returned empty result" - that sounds more like the real problem. Perhaps a bug in FolderLookup?
I guess the best fix is to enhance contract of XMLFileSystem.getAttribute. Passing to Jirka to review and apply the patch.
Created attachment 67384 [details] hg out -p of the change that introduces getAttribute("class:attrName") handling
Reviewers speak up, so that Jirka or me can integrate next week.
Jesse is on vacation I think. DS01: What will happen if clients will code creation method returning not exact type, like this: public static synchronized Action create() ... instead of public static synchronized CreateOnlyOnceAction create()? I think info about class will not be precise then - isn't it a bit of problem - I mean that some isAssignable tests may fail, isn't it right? (I don't know if there are any such places in the system, just theoretically)
Re. DS01, there is a test with Runnable to demonstrate the behaviour. Hopefully not many things fail due to that, but you are right, due to the methodvalue type guessing this is in fact semantically incompatible change.
Ok
VV01: We support C and C++ mime-types and have some actions with constructor MyAction(boolean cpp) we don't want to expose internal Action classes at all, so we have factory methods, but even if we are: public static MyAction craeteCAction() { return MyAction(false);} public static MyAction craeteCppAction() { return MyAction(true);} does it mean that now only one random (the first initialized) action will be instantiated?
Re: VV1, right now your method craeteCAction() is called twice. As well as your craeteCppAction() method. After the fix, each of your methods will be called just once.
Using MQ, or otherwise producing a single patch for review (e.g. using hg diff -r ... -r ...), would be more friendly than forcing reviewers to look over several patches of which the later partially overwrite the former. [JG01] The mistyped overload of methodValue (in both XMLMapAttr and BinaryFS) is confusing. Better would be to make it return Method (or null), and move the call to Method.invoke into the getObject method's body. [JG02] BinaryFS.getType should return Class<?>. Also apichanges should specify that a java.lang.Class object is returned, not a String class name.
Re:[JG01] There is some logic which handles parameters of method. It needs to be duplicated if we want to separate it. Re:[JG02] Agree but apichanges doesn't need to be updated because BinaryFS.AttrImpl is not public API. Otherwise I am going to integrate the patch as it is.
JG01 - it is true that methodValue would in fact need to return a struct of (Method, Object[]). No code duplication is needed; the current call point would call setAccessible and invoke, and the new call point would ignore the Object[] and just use Method.getReturnType. JG02 - sorry for confusion, the "apichanges" I was referring to was openide.filesystems/{apichanges,arch}.xml. BTW this description of behavior should IMO be moved to the XMLFileSystem class Javadoc, not arch.xml, and apichanges.xml should just have a brief mention of the change (leave the details to real documentation).
Re: JG01 - OK, I added MethodAndParams to store Method and Object[]. Please, look at it if I get it right. Re: JG02 - Description moved from arch.xml to XMLFilesystem. If you feel it needs to be more polished, please send me a patch.
Created attachment 68097 [details] Updated patch.
JG01 - clearer, thanks. BTW you can delete getParams(). JG02 - the change I originally requested was to note that the value type is Class, not String.
Don't forget to remove the warning in InstanceDataObject, and close issue #142550, issue #142539, issue #142541, issue #142543, issue #142544, issue #142546, issue #142540, and issue #142836 as INVALID.
The warning shall stay there, imho. It is not printed if the underlaying fs supports the "class:XYZ" semantics, but in case someone would use for example custom SFS extensions, it could still be useful to identify ineffective registrations.
Almost everyone in practice is going to be using XMLFileSystem (in unit tests) and BinaryFS (in the real app). Any warning printed in such a case is just confusing and irritating - filling up the log file with bad advice. In those cases where instanceOf is actually important (i.e. where a correct instanceOf may permit instanceCreate to never be called at all), then the correct attribute to declare is instanceOf, not instanceClass! A warning is acceptable in case (1) instanceOf actually matters (though this is perhaps impossible to check from InstanceDataObject, since it depends on the caller's behavior); (2) instanceOf has not been declared; (3) a strange FileSystem impl is in use which does not support 'class:instanceCreate'.
> Almost everyone in practice is going to be using XMLFileSystem (in unit tests) and BinaryFS (in the real app). Any > warning printed in such a case is just confusing and irritating - filling up the log file with bad advice. The current patch prints no warnings in this cases - because both of these systems support "class:XYZ" extension. So please keep the warning there for the other cases that may happen when people inject their own filesystem implementations into the systemfilesystem.
Integrated (warning not removed). http://hg.netbeans.org/core-main/rev/9d54d7b841c3
The warning can stay for such hypothetical cases but it is still wrong - it should ask them to specify instanceOf, not instanceClass.
Integrated into 'main-golden', available in build *200808251401* on http://bits.netbeans.org/dev/nightly/ Changeset: http://hg.netbeans.org/main/rev/9d54d7b841c3 User: Jiri Skrivanek <jskrivanek@netbeans.org> Log: #131951 - Supress creating of two instances of a class. If you are interested just in the Class of an attribute, but without creating its instance, use fileObject.getAttribute("class:attrName"). This instructs the XMLFileSystem to scan its XML files for definition of attrName attribute and guess its class.
Integrated into 'main-golden', available in build *200808280201* on http://bits.netbeans.org/dev/nightly/ Changeset: http://hg.netbeans.org/main/rev/2de8bea801db User: Jiri Skrivanek <jskrivanek@netbeans.org> Log: #131951 - Warning message should ask for instanceOf instead of instanceClass.
Given this is an incompatible change, shouldn't it be marked as incompatible somewhere? BTW: java.debug is broken due to this, I will likely need to introduce ModuleInstall to fix it.
In what way is this incompatible? How is java.debug broken? Consider filing a separate issue blocking this one.
Yes, please report the incompatibility and let us know. I guess the incompatible change is in loaders, so please create new issue.
From what I know, if a methodvalue references a method which return type is j.l.Object but would return an instance of String, and someone does lookup(String.class) (over the corresponding folder), the lookup result would contain the String from the method before this patch, but wouldn't after this patch. Jarda seems to talk about it in desc32.
That's true, but unavoidable I think if the patch is to work at all. The workaround would simply be to declare <attr name="instanceOf" stringvalue="java.lang.String"/>