Oleksiy Stashok wrote:
> Hi Paul,
>
> here I reimplemented XMLStreamReaderFactory, taking in account the ideas
> you had.
> I think code become more difficult. But may be less hacky.
> Any suggestions?
To the best of my knowledge, there's no need for this extension. We
already provide an opportunity for FI to recycle instances without
touching XMLStreamReaderFactory at all. We invoke
RecycleAware.onRecycled() on everything that implements XMLStreamReader
when it's recycling one. FI's XMLStreamReader could just implement this,
and get notified when it's safe for recycling. FI can maintain its own
factory or pool, and instances can simply go back to it.
In earlier e-mail I've already pointed out that if dependency-wise this
is an issue, we can move these two to stax-ex or istack-commons.
Also, I'm not aware of any caller that really needs the following signature:
doCreate(String systemId, InputStream in, boolean rejectDTDs,
Class<? extends XMLStreamReader> clazz);
Most of the caller knows whether it wants a ordinary XMLStreamReader
that parses Unicode and angle brackets or it wants to parse some other
formats like FI. The caller who needs to switch between FI and text
smartly needs more context information (such as MIME type) anyway before
making up its mind. Such caller can just as well invoke the method on
two different factory classes.
Another problem with this change is that it makes it considerably harder
to implement this extension point and dillutes its semantics. For
example, we'd like BEA to implement this in their own way, so that they
can utilize their own StAX implementation better. Making this class do a
lot more makes it harder, and I don't see any reason why they have to
worry about FI here.
This "have one class do many things" problem shows up evidently in the
bug of the following implementation:
> public XMLStreamReader doCreate(String systemId, InputStream in, boolean rejectDTDs, Class<? extends XMLStreamReader> clazz) {
> try {
> XMLStreamReader xsr = fetch(clazz);
> if(xsr==null)
> return xifMap.get(clazz).createXMLStreamReader(systemId,in);
>
> return xsr;
> } catch (XMLStreamException e) {
> throw new XMLReaderException("stax.cantCreate",e);
> }
> }
when the fetch() succeeds, the codepath doesn't use InputStream at all.
And if you think about it, you'd realize that there's no way for this
implementation class to know how to reset the parser of type 'clazz',
because that's inherently specific to the kind of XMLStreamReader
implementation. The very knowledge we were trying to confine in the
XMLStreamReaderFactory class.
I really suggest FI creates its own pooling mechanism outside this
class. It can implement XMLStreamReaderFactory.RecycleAware, so that FI
XMLStreamReader instances can goes back to its own pool. Such FI factory
can statically-linked to FI's XMLStreamReader implementation, so you can
call any reset/setInputStream method directly.
We really need to roll back this change before we we hit a release,
because then we'll be bound to preserve its compatibility going forward.
--
Kohsuke Kawaguchi
Sun Microsystems kohsuke.kawaguchi_at_sun.com