Jesse, please suggest how to write a test for core/module system. I
can do it, but suggest where to place it. 

The fix could imho be based on addition of new method 
Lookup Lookups.metaInfLookup (ClassLoader l, Lookup previousOne)
that would create new metaInfLookup and inherited all existing
instances from the previous one. I can implement this (easily) in openide.

The core/modules would then use this method to preserve previously
created instances. 

*** Issue 38249 has been marked as a duplicate of this issue. ***

I don't have time to work on this but I can offer some hints. Do not
consider this a P2 bug (or even necessarily a bug at all, except
perhaps a minor performance issue); cf. issue #38249 for explanation.

The proposed added API method would probably be enough. But be careful
to ensure that only instances whose impl classes are loadable from the
new class loader are retained - you do not want to keep around stale
instances from the old module in case a module is disabled and then
quickly reenabled.

Tests for the module system normally go into the existing
ModuleManagerTest. Though this would not test NbTopManager.Lkp.
(Methods in question: sCLC and mCLU.) Could make a new test class in
o.n.c.modules.

Got a better idea. Using one MISL based on MM.SCL is probably not too
good anyway - consider

1. A module can register a service that it cannot itself load (but
that another module can). Weird.

2. If you have two modules with the same package names etc. (e.g. two
incompat versions of an autoload lib), they must both choose different
class names for their MISL instances or they will clash.

Probably better would be to stop using the current Ls.mISL and add a
new factory method to Lookups:

public interface MetaInfServiceFinder {
    Enumeration/*<URL>*/ findServices(String className);
}
public Lookup metaInfServicesLookup(ClassLoader l,
MetaInfServiceFinder f);

Then NbTM.Lkp should be modified to create one MISL *per module*. It
could go through the list of enabled modules during startup, scanning
each module JAR (Module.jarFile) for META-INF/services/* entries
(skipping any modules with no such entries) and calling the new
factory; the finder would look directly in the JAR file and return
jar: URLs. When a module is disabled, remove its lookup (if any); when
enabled, scan it.

It does not suffice to do this trick using the current factory method
because if module A declares service S1 and module B depends on A and
declares service S2, B's class loader will find both S1 and S2,
creating duplicates. You need to scan b.jar directly.

Such a change might have beneficial performance impact too, as it
would avoid calling MM.SCL.findAllResources(String). The finder impl
could prescan the JAR for all known entries so that
findServices(String) would be fast.

What is surprising that the similar test is in

org.netbeans.core.lookup.MetaInfLookupTest

and it is passing fine.

Still reproducible.
Do you really test Lookup.getDefault().lookup(Debugger.class);
based on mf-layers.xml?
    <folder name="Services">        
        <folder name="Hidden">
            <file name="debugger.instance">
                <attr name="instanceClass"
stringvalue="org.netbeans.modules.debugger.multisession.EnterpriseDebugger"/>
            </file>
        </folder>

See my mflayer here:
trunk\debuggercore\src\org\netbeans\modules\debugger\resources\mf-layer.xml

Aha, you are registering it in a layer, not in META-INF/services! That
is quite different. So perhaps META-INF/services already works fine
(which would be one quick fix for #38249 to use - anyway using
META-INF/services is preferred for singletons like this).

So the problem is presumably in InstanceDataObject, or perhaps in
FolderLookup.

Of course I am not testing debugger - that is part of issue 38249, I
am looking for a simple test how to reproduce the problem
automatically. If you know how to write automated test for Debugger,
please contribute. Otherwise I am going to try layer based
registration similar to what you pointed out in
org.netbeans.core.lookup.MetaInfLookupTest, but not exact.

Test written that tries to reproduce the problem using .instance in
layer file, but the behaviour could not be simulated. Closing as invalid.

http://www.netbeans.org/source/browse/core/test/unit/src/org/netbeans/core/lookup/InstanceDataObjectModule38420Test.java

Yarda,
its not invalid, its still reproducible.
Its very easy to reproduce it in Netbeans + debugger.
Attach to NB from debugger and evaluate org.openide..Lookup.getDefault
().lookup (org.netbeans...Debugger.class);

And regarding your testcase. Is not a difference that you keep
instance of Lookup.Result?

Or.. can you create test exactly on Debugger.class instance?

Nice comment, but more appropriate for 38249. What exactly makes you
believe that the problem is in core? You should try really harder to
blame us for your problems. 

I've connected to ide, printed the expression and got syntax error.
But never mind, that should still prove nothing. Even if I got one
instance,  then disable module and got another one that still does not
prove anything. That is the regular behaviour. 

You have to show us that there are really two instances of debugger in
memory at once and that they were both created by Lookup.getDefault.
Otherwise you are not describing bug in core.

Still reproducible.
I have added folowing code to EnterpriseDebugger  (package
org.netbeans.modules.debugger.multisession):
    {
        Thread.dumpStack();
    }
    
    protected void finalize () {
        System.out.println("EnterpriseDebugger.finally");
    }

And second instance of Enterprise Debugger has been created when I
have reinstalled Welcome Screen module.

Created attachment 12855 [details]
And old instance has not been released.

Hanzi, this is excelent investigation thank you. We have tracked the
problem more thru FolderLookup and
InstanceDataObject.Ser.instanceCreate and find out that the sinner is
BinaryFS or module manager that changes the FileObject.lastModified()
time of your settings file when welcome screen module is enabled/disabled.

Reassigning to core/modules.

Petr do you feel like taking this? :-)

I had been affraid you'll ask...
Some comments:
The problem is not specific to BinaryFS, the same thing happens when
you use all-layers.xml caching as there is only one timestamp in both
cases.
In BinaryFS, it is a bit more complicated, as there are MLFS delegates
replaced and I'm not sure how would the MFS react.

Generally, I'm not sure on correct behaviour. Is it a bug if the
InstanceDO recreates the instance without direct reason. How does the
clients recognize it is "without direct reason"
Is user just touching a layer file a direct reason.
Should the debugger (or any other client) break just because user
touched a file?

It's definitely not acceptable for the debugger to break just because
someone touches a file defined a layer - that is why issue #38249 is
filed separately. BTW I am removing MERCURY kw from this bug and
downgrading it because the bug the user cares about is #38249, which
can (and should!) be fixed independently of this issue, which I
consider an RFE for a more conservative object recreation policy
(potentially helpful for performance).

The problem with the timestamp is that InstanceDataObject assumes that
if the timestamp of an instance file varies from call to call that the
file contents must have changed - significantly enough to warrant
producing a new instance. However a layer-based file of course has no
well-defined timestamp so this assumption is a poor one. We could
perhaps always return 0L as the lastmod timestamp for any layer-based
file and hope for the best.

Jesse, 
I do not care about implementation details. I would like to see some
definition how Lookup works. So, as I understand it you are not able
to guarantee that Lookup will work correctly in described case. OK, I
will find some other solution. But, be so kind and mention it in
JavaDoc for Lookup, please.

I think that Hanz is right "the definition how Lookup works" should be
somewhere. And as far as I know a maximum effort has been taken to
construct only one instance for Lookup.Item. MetaInfLookup does that.
FolderLookup keeps the DataObject and delegates to it.
InstanceDataObject keeps the same instance if the file does not
change. Moreover I believe that the tests in
core/test/unit/src/org/netbeans/core/lookup check for this behaviour
as well. That is why I believe the requested behaviour is clear, it is
broken and it is serious problem. That is why I made this P2 again. We
should fix it asap, I am relatively bugs-free, so I can do it with a
little help of you.

As I said I think there already is a test for this, but it seems that
the test runs in  some kind of artificial environment and not in the
real IDE state (Plain top manager, memoryfs). Please help me simulate
the real state and I will fix the problem.

Btw. 1st idea I had is to run the tests in IDE mode, but the install
of modules failed. I have not investigated further.

Btw. 2nd idea would be to initialize the (binary) caching for the
test, but I rely on your guidance to tell me how.

OK, if you think we should guarantee this Lookup semantics, fine with me.

Re. writing a test that would cover caching: I'm not sure offhand what
you need to do. You may need to restart core at least once (which
would be a huge headache for a test), but maybe not. Probably
netbeans.user must be set. Most of the logic which is relevant for
getting the test to actually test caches is going to be in
ModuleLayeredFileSystem; run with -Dorg.netbeans.core.projects=0 to
trace what is going on.

Now the problem can be reproduced. Just run:

ant -f core/test/build.xml -Dxtest.attribs=cache

to simulate the problem.


Checking in bootstrap/src/org/netbeans/JarClassLoader.java;
/cvs/core/bootstrap/src/org/netbeans/JarClassLoader.java,v  <-- 
JarClassLoader.java
new revision: 1.13; previous revision: 1.12
done
Processing log script arguments...
More commits to come...
Checking in test/cfg-unit.xml;
/cvs/core/test/cfg-unit.xml,v  <--  cfg-unit.xml
new revision: 1.51; previous revision: 1.50
done
Processing log script arguments...
More commits to come...
Checking in
test/unit/src/org/netbeans/core/lookup/InstanceDataObjectModuleTest7.java;
/cvs/core/test/unit/src/org/netbeans/core/lookup/InstanceDataObjectModuleTest7.java,v
 <--  InstanceDataObjectModuleTest7.java
new revision: 1.7; previous revision: 1.6
done
Checking in
test/unit/src/org/netbeans/core/lookup/InstanceDataObjectModuleTestHid.java;
/cvs/core/test/unit/src/org/netbeans/core/lookup/InstanceDataObjectModuleTestHid.java,v
 <--  InstanceDataObjectModuleTestHid.java
new revision: 1.6; previous revision: 1.5
done
Checking in
test/unit/src/org/netbeans/core/lookup/LookupClassLoaderTest.java;
/cvs/core/test/unit/src/org/netbeans/core/lookup/LookupClassLoaderTest.java,v
 <--  LookupClassLoaderTest.java
new revision: 1.5; previous revision: 1.4
done
Checking in
test/unit/src/org/netbeans/core/lookup/MetaInfServicesTest.java;
/cvs/core/test/unit/src/org/netbeans/core/lookup/MetaInfServicesTest.java,v
 <--  MetaInfServicesTest.java
new revision: 1.8; previous revision: 1.7
done

Created attachment 13024 [details]
Proposed fix

I am ready to commit the patch as it seems to fix the original
problem. It is based on proper (better) implementation of
FileObject.lastModified() for BinaryFS. I've enhanced the
CacheManagerTest to verify the correct behaviour of lastModified, but
I have not succeed to implement correct behaviour for all the existing
caches: BinaryFS and NonCaching differ in one case, XMLLayer cache is
completely broken. I would suggest to delete XMLLayerCacheManager as
it is obsoleted and I could fix the NonCaching and BinaryFS difference
if somebody tells me what is the correct behavior. But this probably
does not prevent the integration.

Format of Binary cache file has been enhanced to list all base URLs
(usually references to xml layer files) at the begining and each
MemFile has an integer pointer to the list.

Not sure what

    lastModified = -10 - base;

is supposed to do; needs a comment I think. :-)

The patch looks OK, though I am not sure I understand why we even care
about lastModified for layer files. Can't you just always return 0L
for *any* file in XMLFileSystem or BinaryFS? That would certainly
ensure that the timestamp did not change during a cache recreation.
Who really cares when the layer.xml was last changed anyway? Is it
necessary for lastModified to change when you reload test modules, or
is it enough that disabling the module fires a file removal and
reenabling it fires a new file creation?

Re. XMLLayerCacheManagerImpl - it's true it should not be used any
more. It is still potentially useful to have around in case you want
to get a dump of merged layers for debugging purposes. I wouldn't be
upset if it were deleted.

lastModified is cache. When < 0 it is unitialized, when >= 0 it
contains the time. -10 could be any other suitable constant.

I was affraid of returning OL as this would not work when module is
reinstalled, its new version is downloaded. Then the resource file
could change, but the lastModified time would still remain the same.
That is why I implemented the solution that usually uses time from
some "real" files.

I'll delete the XMLLayerCacheManagerImpl and apply the patch tomorrow.

/cvs/openide/src/org/openide/filesystems/XMLFileSystem.java,v  <-- 
XMLFileSystem.java
new revision: 1.73; previous revision: 1.72
done
Processing log script arguments...
More commits to come...
Checking in
core/src/org/netbeans/core/projects/cache/BinaryCacheManager.java;
/cvs/core/src/org/netbeans/core/projects/cache/BinaryCacheManager.java,v
 <--  BinaryCacheManager.java
new revision: 1.7; previous revision: 1.6
done
Checking in core/src/org/netbeans/core/projects/cache/BinaryFS.java;
/cvs/core/src/org/netbeans/core/projects/cache/BinaryFS.java,v  <-- 
BinaryFS.java
new revision: 1.8; previous revision: 1.7
done
Checking in
core/src/org/netbeans/core/projects/cache/ParsingLayerCacheManager.java;
/cvs/core/src/org/netbeans/core/projects/cache/ParsingLayerCacheManager.java,v
 <--  ParsingLayerCacheManager.java
new revision: 1.9; previous revision: 1.8
done
Removing
core/src/org/netbeans/core/projects/cache/XMLLayerCacheManagerImpl.java;
/cvs/core/src/org/netbeans/core/projects/cache/XMLLayerCacheManagerImpl.java,v
 <--  XMLLayerCacheManagerImpl.java
new revision: delete; previous revision: 1.6
done
Processing log script arguments...
More commits to come...
Checking in core/test/cfg-unit.xml;
/cvs/core/test/cfg-unit.xml,v  <--  cfg-unit.xml
new revision: 1.53; previous revision: 1.52
done
Processing log script arguments...
More commits to come...
Checking in
core/test/unit/src/org/netbeans/core/lookup/InstanceDataObjectModule38420Test.java;
/cvs/core/test/unit/src/org/netbeans/core/lookup/InstanceDataObjectModule38420Test.java,v
 <--  InstanceDataObjectModule38420Test.java
new revision: 1.2; previous revision: 1.1
done
Checking in
core/test/unit/src/org/netbeans/core/lookup/InstanceDataObjectModuleTestHid.java;
/cvs/core/test/unit/src/org/netbeans/core/lookup/InstanceDataObjectModuleTestHid.java,v
 <--  InstanceDataObjectModuleTestHid.java
new revision: 1.7; previous revision: 1.6
done
Processing log script arguments...
More commits to come...
Checking in
core/test/unit/src/org/netbeans/core/projects/cache/BinaryCacheManagerTest.java;
/cvs/core/test/unit/src/org/netbeans/core/projects/cache/BinaryCacheManagerTest.java,v
 <--  BinaryCacheManagerTest.java
new revision: 1.5; previous revision: 1.4
done
Checking in
core/test/unit/src/org/netbeans/core/projects/cache/CacheManagerTestBaseHid.java;
/cvs/core/test/unit/src/org/netbeans/core/projects/cache/CacheManagerTestBaseHid.java,v
 <--  CacheManagerTestBaseHid.java
new revision: 1.5; previous revision: 1.4
done
Checking in
core/test/unit/src/org/netbeans/core/projects/cache/NonCacheManagerTest.java;
/cvs/core/test/unit/src/org/netbeans/core/projects/cache/NonCacheManagerTest.java,v
 <--  NonCacheManagerTest.java
new revision: 1.2; previous revision: 1.1
done
Removing
core/test/unit/src/org/netbeans/core/projects/cache/XMLLayerCacheManagerImplTest.java;
/cvs/core/test/unit/src/org/netbeans/core/projects/cache/XMLLayerCacheManagerImplTest.java,v
 <--  XMLLayerCacheManagerImplTest.java
new revision: delete; previous revision: 1.4
done

fixed by reporter - verified