[jboss-dev] VFS Temp Reuse (JBAS-6715)

[Jason T. Greene]

Ales Justin wrote:
>>> Why exactly is ZEC-B created?
>>> It should be only one, as that's the point of having a cache.
>>
>> It looks like nested zip entries are never registered in the cache 
>> (see mountZipStream) in ZipEntryContext.
> 
> I still don't see how that could happen.
> 
> As the only api to get a VirtualFile is over VFS class.
> And VFS goes over VFSRegistry, which knows about the cache.
> 
> New mounted ZEC should be a child of some already cached context.
> And when someone asks for URL --> VirtualFile,
> it should actually hit that parent context,
> (see your newly added VFSContext::getFurthestParentTemp(String path))
> which would then know about that newly mounted ZEC.

Ah, yeah, it definitely does not the ear root should be registered at 
this point in time. I'll see if I can figure out why it isn't.

ZipEntryContext.mountZipFile(ZipEntryContext.java:668)
ZipEntryContext.initEntries(ZipEntryContext.java:559)
ZipEntryContext.ensureEntries(ZipEntryContext.java:612)
ZipEntryContext.checkIfModified(ZipEntryContext.java:774)
ZipEntryContext.getChild(ZipEntryContext.java:818)
ZipEntryHandler.createChildHandler(ZipEntryHandler.java:191)
AbstractVirtualFileHandler.structuredFindChild(AbstractVirtualFileHandler.java:684)
ZipEntryHandler.getChild(ZipEntryHandler.java:165)
DelegatingHandler.getChild(DelegatingHandler.java:107)
AbstractVirtualFileHandler.structuredFindChild(AbstractVirtualFileHandler.java:689)
FileHandler.getChild(FileHandler.java:302)
DefaultVFSRegistry.findHandler(DefaultVFSRegistry.java:104)
DefaultVFSRegistry.getFile(DefaultVFSRegistry.java:87)
DefaultVFSRegistry.getFile(DefaultVFSRegistry.java:120)
AbstractVFSHandler.openConnection(AbstractVFSHandler.java:71)


>>> If it was only a single instance, as the cache presumes,
>>> there should be no problem, as the entity is reset no cleanup.
>>
>> That would definitely be more efficient, but what if the cache evicts 
>> the entry (lru etc)?
> 
> By keeping a VirtualFile ref,
> you should be aware that it might be cleaned.
> 
> The only time we currently do it, is at undeploy,
> and the only ref we keep is the top deployment root.
> If things are properly reset, there should not be a problem.
> 
> I agree it's not a bullet proof concept,
> but I doubt we need to impl GC like functionality for this usecase.

Well, I mean that the underlying Registry is backed by cache that 
expires entries. So if the design requires that there is only one 
context instance, then it will eventually break. As an example say that 
you deploy something, time passes, the cache entry is evicted, then you 
go to redeploy.

So I think we have to either switch to a permanent registry or do some 
kind of GC.

Another possible workaround that would force cleanup, would be to allow 
a tempinfo to refer to multiple handles, then if it is reused, all 
duplicate contexts are cleaned.

> 
>> That would also mean making sure that the deployment code never holds 
>> on to a ref during redeploy.
> 
> That would be the simplest workaround,
> but I would still like to see VFS's cleanup doing things properly,
> what ever that might eventually be - which impl, ...

Yeah I agree. Otherwise we could leak refs, temps etc.

> 
>> I can think of two potential ways to solve this, although both would 
>> exceed the timeline we need to get this fixed by (last week :)
>>
>> 1. We could cleanup just the calling handler's temp, and refcount the 
>> temp (although this would require that people call cleanup on virtual 
>> file entries)
> 
> This would never work,
> as you don't have all the handles available at cleaup
> or/and people would have to code against a notion of doing a cleanup.
>

Yeah bad idea at 3 am :)

>> 2. We could make VirtualFile refs rediscover themselves after they are 
>> cleaned
> 
> This is probably doable,
> but going backwards on a structure like nested zips is a pita.
> (been there, done that :-))
> 
> e.g. if you wanna keep things minimal + have valid fs structure
> you would have to somehow change or reuse the existing parent refs

It does seem like alot of work, but it might be a good future direction 
(maybe 3.x type stuff).

>> Well, I am exhausted, so I am going to crash. 
> 
> Having nightmares about the VFS? :-)

LOL

> 
>> I just committed the JBVFS-108 changes. In case you have trouble 
>> reproducing on Windows, it
>> 's easy to find if you modify ZipEntryContext.getChild to check if the 
>> child's temp is still registered and dump an error if it is not.
> 
> I'll try to see why that ZEC-B is created.
> 
> Or if you could provide me with your usecase,
> either as instructions on how that new creation happens
> or putting it directly into VFS's tests.

OK. I think I have enough info to mock this (just need to nail down the 
missing root context cache entry). I will also if the above workaround 
has a chance of solving this.

-- 
Jason T. Greene
JBoss, a division of Red Hat