XFire

Concurrency issue in StaxUtils.createXMLStreamReader

Details

  • Type: Bug Bug
  • Status: Open Open
  • Priority: Major Major
  • Resolution: Unresolved
  • Affects Version/s: 1.2.6
  • Fix Version/s: 1.2.7
  • Component/s: Core
  • Labels:
    None
  • Environment:
    Windows XP, Java 5 update 9
  • Number of attachments :
    2

Description

I've created a simple HelloWorld service that echoes a name back to the client. This works fine until i create 2 or more threads that calls the service at the same time. Every now and then, I get an XML parsing error. The content of the request is always correct however and making this small change to StaxUtils fixes the issue.

759 synchronized(factory)

{ 760 return factory.createXMLStreamReader(in, encoding); 761 }

So this would indicate that it is not safe to use the same XMLInputFactory instance in a multi-threaded way. Obviously synchronisation is not an optimal solution, but hopefully you guys can think of something better .

  1. Java Source File
    HelloWorldClient.java
    22/May/07 12:30 PM
    3 kB
    Walter Seymore
  2. Java Source File
    HelloWorldService.java
    22/May/07 12:30 PM
    0.1 kB
    Walter Seymore

Activity

Ascending order - Click to sort in descending order
Hide
Permalink
Holger Hoffstätte added a comment -

I can't say anything about the createXML.. factory methods (parser-dependent detail?) but both getXMLInputFactory(MessageContext ctx) and getXMLOutputFactory(MessageContext ctx) are not threadsafe either.
In both cases the if (..instanceof..) blocks that access the factories Map need to be synchronized on a shared lock.

Show
Holger Hoffstätte added a comment - I can't say anything about the createXML.. factory methods (parser-dependent detail?) but both getXMLInputFactory(MessageContext ctx) and getXMLOutputFactory(MessageContext ctx) are not threadsafe either. In both cases the if (..instanceof..) blocks that access the factories Map need to be synchronized on a shared lock.
Hide
Permalink
Tatu Saloranta added a comment -

For what it's worth, Woodstox factories are fully thread-safe in regards to reader/writer construction methods (as well as wrt. any other interaction between factory and readers/writers it creates). There is no need for synchronization; although at the same time, cost of synchronization is rather small relative to all xml processing. Constructing readers and writers is a cheap operation.
I think this is true for other Stax implementations as well, although to be sure you might want to contact Sun sjsxp folks, or check out source code.

The only non-thread-safe part of dealing with factories is configuration: changing settings via factory.setProperty() is not (guaranteed to be) thread-safe.

Show
Tatu Saloranta added a comment - For what it's worth, Woodstox factories are fully thread-safe in regards to reader/writer construction methods (as well as wrt. any other interaction between factory and readers/writers it creates). There is no need for synchronization; although at the same time, cost of synchronization is rather small relative to all xml processing. Constructing readers and writers is a cheap operation. I think this is true for other Stax implementations as well, although to be sure you might want to contact Sun sjsxp folks, or check out source code. The only non-thread-safe part of dealing with factories is configuration: changing settings via factory.setProperty() is not (guaranteed to be) thread-safe.
Hide
Permalink
Tatu Saloranta added a comment -

Hmmmh. Code in STAXUtils is bit of a mess... I'm actually not quite sure why proposed synchronization would solve problems, as it would seem to be more of a side-effect.
Method configureFactory() looks bit suspicious all in all, as I'm pretty sure the configuration part should be synchronized.

Show
Tatu Saloranta added a comment - Hmmmh. Code in STAXUtils is bit of a mess... I'm actually not quite sure why proposed synchronization would solve problems, as it would seem to be more of a side-effect. Method configureFactory() looks bit suspicious all in all, as I'm pretty sure the configuration part should be synchronized.
Hide
Permalink
Walter Seymore added a comment -

I have swapped in the Woodstox parser and that fixed the problem too. Thanks! This is not in the spec though (https://sjsxp.dev.java.net/issues/show_bug.cgi?id=4), so this might happen with any other parser too.

Show
Walter Seymore added a comment - I have swapped in the Woodstox parser and that fixed the problem too. Thanks! This is not in the spec though ( https://sjsxp.dev.java.net/issues/show_bug.cgi?id=4 ), so this might happen with any other parser too.
Hide
Permalink
Tatu Saloranta added a comment -

Hmmh. This is unfortunate – I was hoping sjsxp didn't try to be too smart. But I think that it does try to actually reuse stream reader instances, which (in my opinion) is not the best level at which to reuse objects, and leads to problems like this.
But even if it does that, it should (once again, IMO) do synchronization at factory level, given that resulting problems are rather hard to track down otherwise. Finally, the behavior could of course be documented... except that Sjsxp is kind of hidden within JDK, so there's no good place to add such notes for users.

Anyway, it is true that implementations are not required to be thread-safe. I wish specification did mandate thread-safety; but fortunately cost of synchronizing on input factory for construction is relatively inexpensive. So perhaps XFire needs to add that synchronization given Sjsxp behavior, and the fact it's bundled with Java 6.

Show
Tatu Saloranta added a comment - Hmmh. This is unfortunate – I was hoping sjsxp didn't try to be too smart. But I think that it does try to actually reuse stream reader instances, which (in my opinion) is not the best level at which to reuse objects, and leads to problems like this. But even if it does that, it should (once again, IMO) do synchronization at factory level, given that resulting problems are rather hard to track down otherwise. Finally, the behavior could of course be documented... except that Sjsxp is kind of hidden within JDK, so there's no good place to add such notes for users. Anyway, it is true that implementations are not required to be thread-safe. I wish specification did mandate thread-safety; but fortunately cost of synchronizing on input factory for construction is relatively inexpensive. So perhaps XFire needs to add that synchronization given Sjsxp behavior, and the fact it's bundled with Java 6.
Hide
Permalink
Jimmy Van Broeck added a comment -

I've got the same problem, but even with a greater consequence.

Sometimes a thread was in a looped state, because of this and ate all the resources of my server.

In some cases the Stax parser came in a bad state and went into an endless loop. (middle of the document and fscannerstate on 7 (Start_document)

Problems lays in the factory.createXMLStreamReader. This function resets the inputstream and the streamreader. One of the things is that it resets the scannerstate to 7 and here it the problem.

So i made the createXMLStreamReader synchronized and the problem is gone.

Show
Jimmy Van Broeck added a comment - I've got the same problem, but even with a greater consequence. Sometimes a thread was in a looped state, because of this and ate all the resources of my server. In some cases the Stax parser came in a bad state and went into an endless loop. (middle of the document and fscannerstate on 7 (Start_document) Problems lays in the factory.createXMLStreamReader. This function resets the inputstream and the streamreader. One of the things is that it resets the scannerstate to 7 and here it the problem. So i made the createXMLStreamReader synchronized and the problem is gone.
Hide
Permalink
Arno van de Kamp added a comment -

The proposed fix seems ok in general. We have the same problems in our live production system as Jimmy. We hope you will implement the fix in 1.2.7 soon.

Show
Arno van de Kamp added a comment - The proposed fix seems ok in general. We have the same problems in our live production system as Jimmy. We hope you will implement the fix in 1.2.7 soon.

People

  • Assignee:
    Dan Diephouse
    Reporter:
    Walter Seymore
Vote (4)
Watch (5)

Dates

  • Created:
    Updated: