Bug 131951 - Menu action registered in layer has two instances
Menu action registered in layer has two instances
Status: RESOLVED FIXED
Product: platform
Classification: Unclassified
Component: Filesystems
6.x
All All
: P2 (vote)
: 6.x
Assigned To: Jiri Skrivanek
issues@platform
: API_REVIEW_FAST
Depends on: 141682 142550 145641
Blocks: 142540 142836
  Show dependency treegraph
 
Reported: 2008-04-03 15:53 UTC by Jiri Sedlacek
Modified: 2008-12-22 14:05 UTC (History)
6 users (show)

See Also:
Issue Type: DEFECT
:


Attachments
Simple NetBeans Platform Application reproducing the problem (11.79 KB, application/octet-stream)
2008-04-03 15:54 UTC, Jiri Sedlacek
Details
thread dumps of instantiations, which explains what is going on (19.29 KB, text/plain)
2008-08-06 15:40 UTC, David Simonek
Details
hg out -p of the change that introduces getAttribute("class:attrName") handling (34.24 KB, patch)
2008-08-14 09:42 UTC, Jaroslav Tulach
Details | Diff
Updated patch. (27.06 KB, text/plain)
2008-08-22 10:55 UTC, Jiri Skrivanek
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Jiri Sedlacek 2008-04-03 15:53:09 UTC
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.
Comment 1 Jiri Sedlacek 2008-04-03 15:54:07 UTC
Created attachment 59627 [details]
Simple NetBeans Platform Application reproducing the problem
Comment 2 Jiri Sedlacek 2008-04-03 15:59:44 UTC
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
Comment 3 Jaroslav Tulach 2008-04-04 14:23:00 UTC
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...
Comment 4 Jiri Sedlacek 2008-04-04 14:26:08 UTC
No I don't, take your time to investigate:o) I have to use 6.0.1 platform for now, anyway.
Comment 5 Antonin Nebuzelsky 2008-07-21 10:43:37 UTC
Dafe, please take this one.
Comment 6 David Simonek 2008-07-22 17:41:06 UTC
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.
Comment 7 David Simonek 2008-07-24 12:45:18 UTC
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.
Comment 8 David Simonek 2008-07-31 12:22:47 UTC
Reopening, I have to rollback the fix, it caused P1 - 141682
Comment 9 David Simonek 2008-07-31 12:31:27 UTC
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.
Comment 10 David Simonek 2008-07-31 12:37:33 UTC
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
Comment 11 Quality Engineering 2008-08-01 04:14:33 UTC
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)
Comment 12 Lukas Hasik 2008-08-01 08:48:49 UTC
I'll report all the warnings to appropriate components
Comment 13 Jesse Glick 2008-08-01 21:05:34 UTC
[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.
Comment 14 Jiri Sedlacek 2008-08-04 13:50:34 UTC
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.
Comment 15 Jesse Glick 2008-08-04 19:02:01 UTC
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.
Comment 16 David Simonek 2008-08-06 14:17:06 UTC
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.
Comment 17 Jesse Glick 2008-08-06 15:03:05 UTC
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.
Comment 18 David Simonek 2008-08-06 15:40:05 UTC
Created attachment 66707 [details]
thread dumps of instantiations, which explains what is going on
Comment 19 David Simonek 2008-08-06 15:44:46 UTC
After off line discussion with jtulach, I'm passing back to him for better fix. Sorry I didn't come with something
better myself...
Comment 20 David Simonek 2008-08-06 15:48:13 UTC
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.
Comment 21 Jesse Glick 2008-08-07 00:06:54 UTC
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.)
Comment 22 David Simonek 2008-08-07 08:03:10 UTC
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.
Comment 23 Jaroslav Tulach 2008-08-07 10:12:15 UTC
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.
Comment 24 Jesse Glick 2008-08-07 21:58:18 UTC
#80eea651d4d9 looks right to me. What was the problem? "Too soon" for whom exactly?
Comment 25 David Simonek 2008-08-08 11:57:20 UTC
"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.
Comment 26 Jesse Glick 2008-08-08 14:43:34 UTC
"Lookup.forPath("some valid path").lookupAll(Object.class) returned empty result" - that sounds more like the real
problem. Perhaps a bug in FolderLookup?
Comment 27 Jaroslav Tulach 2008-08-14 09:37:22 UTC
I guess the best fix is to enhance contract of XMLFileSystem.getAttribute. Passing to Jirka to review and apply the 
patch.
Comment 28 Jaroslav Tulach 2008-08-14 09:42:41 UTC
Created attachment 67384 [details]
hg out -p of the change that introduces getAttribute("class:attrName") handling
Comment 29 Jaroslav Tulach 2008-08-14 09:43:46 UTC
Reviewers speak up, so that Jirka or me can integrate next week.
Comment 30 David Simonek 2008-08-14 15:39:02 UTC
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) 
Comment 31 Jaroslav Tulach 2008-08-15 09:06:10 UTC
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.
Comment 32 David Simonek 2008-08-15 10:42:02 UTC
Ok
Comment 33 Vladimir Voskresensky 2008-08-17 10:15:00 UTC
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?
Comment 34 Jaroslav Tulach 2008-08-18 16:04:34 UTC
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.
Comment 35 Jesse Glick 2008-08-18 20:55:06 UTC
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.
Comment 36 Jiri Skrivanek 2008-08-21 14:04:34 UTC
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.
Comment 37 Jesse Glick 2008-08-21 17:35:46 UTC
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).
Comment 38 Jiri Skrivanek 2008-08-22 10:54:28 UTC
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.
Comment 39 Jiri Skrivanek 2008-08-22 10:55:03 UTC
Created attachment 68097 [details]
Updated patch.
Comment 40 Jesse Glick 2008-08-22 15:27:07 UTC
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.
Comment 41 Jesse Glick 2008-08-22 15:31:12 UTC
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.
Comment 42 Jaroslav Tulach 2008-08-22 16:39:17 UTC
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.
Comment 43 Jesse Glick 2008-08-22 17:10:02 UTC
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'.
Comment 44 Jaroslav Tulach 2008-08-24 06:47:48 UTC
> 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.
Comment 45 Jiri Skrivanek 2008-08-25 10:06:06 UTC
Integrated (warning not removed).
http://hg.netbeans.org/core-main/rev/9d54d7b841c3
Comment 46 Jesse Glick 2008-08-25 15:29:24 UTC
The warning can stay for such hypothetical cases but it is still wrong - it should ask them to specify instanceOf, not
instanceClass.
Comment 47 Quality Engineering 2008-08-25 17:25:17 UTC
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.
Comment 48 Quality Engineering 2008-08-28 06:42:10 UTC
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.
Comment 49 Jan Lahoda 2008-08-29 16:06:46 UTC
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.
Comment 50 Jesse Glick 2008-08-29 16:12:48 UTC
In what way is this incompatible? How is java.debug broken? Consider filing a separate issue blocking this one.
Comment 51 Jaroslav Tulach 2008-08-29 19:41:55 UTC
Yes, please report the incompatibility and let us know. I guess the incompatible change is in loaders, so please 
create new issue.
Comment 52 Jan Lahoda 2008-08-29 19:44:00 UTC
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.
Comment 53 Jesse Glick 2008-08-29 19:52:08 UTC
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"/>


By use of this website, you agree to the NetBeans Policies and Terms of Use. © 2012, Oracle Corporation and/or its affiliates. Sponsored by Oracle logo