concurrency issue with Factory?

[tpeierls]

I just stumbled across the new Factory class in org.directwebremoting.extend and noticed a couple of problems. The very minor one is that it seems to have been generified from code that was specific to JsonParser, e.g., get(ServletContext) is documented as returning "The current JsonParser".

The more interesting one is that the builder field (and thus the entire class) is not safe for concurrent access. It might be that instances of Factory are meant to be confined to a single thread, in which case the documentation should reflect that. If not, though, builder should be made a volatile or be guarded by a lock.

--tim

[Joe Walker-3]

On Sun, Aug 31, 2008 at 5:34 PM, Tim Peierls <tim@...> wrote:

I just stumbled across the new Factory class in org.directwebremoting.extend and noticed a couple of problems. The very minor one is that it seems to have been generified from code that was specific to JsonParser, e.g., get(ServletContext) is documented as returning "The current JsonParser".

Yeah - the JsonParser was the point were my happiness to cut and paste broke, and I generalized it. Thanks.
 

The more interesting one is that the builder field (and thus the entire class) is not safe for concurrent access. It might be that instances of Factory are meant to be confined to a single thread, in which case the documentation should reflect that. If not, though, builder should be made a volatile or be guarded by a lock.

Once again I'm slightly humbled and scared by the frequency with which you find these.
I've added to the documentation that the function should only be called while the context is starting up.
I believe that the setter should be called first, once only, and from then on it will only be read.

Joe.

[tpeierls]

I don't know enough about how the Factory class is used, but if there's any possibility that a thread other than the one that calls setBuilder will try to read the value of the builder field, then you need to either make builder volatile or guard it with some sort of synchronization. Otherwise that reader thread might see a null value even if the read happens a long time after the call to setBuilder.

I only noticed this because Patrick Lightbody mentioned the existence of ServerContextFactory.get() with no arguments -- something I hadn't known about -- and I wanted to see how it worked.

--tim

Joe Walker-3 wrote:

On Sun, Aug 31, 2008 at 5:34 PM, Tim Peierls <[e-mail]> wrote:

> I just stumbled across the new Factory class in
> org.directwebremoting.extend and noticed a couple of problems. The very
> minor one is that it seems to have been generified from code that was
> specific to JsonParser, e.g., get(ServletContext) is documented as returning
> "The current JsonParser".
>

Yeah - the JsonParser was the point were my happiness to cut and paste
broke, and I generalized it. Thanks.


> The more interesting one is that the builder field (and thus the entire
> class) is not safe for concurrent access. It might be that instances of
> Factory are meant to be confined to a single thread, in which case the
> documentation should reflect that. If not, though, builder should be made a
> volatile or be guarded by a lock.
>

Once again I'm slightly humbled and scared by the frequency with which you
find these.
I've added to the documentation that the function should only be called
while the context is starting up.
I believe that the setter should be called first, once only, and from then
on it will only be read.

Joe.

[tpeierls]

I see that Joe addressed this by making the builder field volatile.

There's a slight improvement that could be made, however. As the code stands, the builder field is accessed twice in each get method, once to check for nullity and then again to use the non-null value.

It would better to read the volatile field once into a temporary variable. It's more efficient and it prevents the (extremely unlikely) case where the value is set to null after being seen as non-null.

I have the patch ready to check in, if you like, but it's just adding this line (in two methods) before the nullity test:

    Builder<T> builder = this.builder;

--tim

tpeierls wrote:

I don't know enough about how the Factory class is used, but if there's any possibility that a thread other than the one that calls setBuilder will try to read the value of the builder field, then you need to either make builder volatile or guard it with some sort of synchronization. Otherwise that reader thread might see a null value even if the read happens a long time after the call to setBuilder.

I only noticed this because Patrick Lightbody mentioned the existence of ServerContextFactory.get() with no arguments -- something I hadn't known about -- and I wanted to see how it worked.

--tim

[Joe Walker-3]

That's fine, Tim - go for it.
Joe.

On Fri, Sep 5, 2008 at 8:36 PM, tpeierls <tim@...> wrote:

I see that Joe addressed this by making the builder field volatile.

There's a slight improvement that could be made, however. As the code
stands, the builder field is accessed twice in each get method, once to
check for nullity and then again to use the non-null value.

It would better to read the volatile field once into a temporary variable.
It's more efficient and it prevents the (extremely unlikely) case where the
value is set to null after being seen as non-null.

I have the patch ready to check in, if you like, but it's just adding this
line (in two methods) before the nullity test:

   Builder<T> builder = this.builder;

--tim

tpeierls wrote:
>
> I don't know enough about how the Factory class is used, but if there's
> any possibility that a thread other than the one that calls setBuilder
> will try to read the value of the builder field, then you need to either
> make builder volatile or guard it with some sort of synchronization.
> Otherwise that reader thread might see a null value even if the read
> happens a long time after the call to setBuilder.
>
> I only noticed this because Patrick Lightbody mentioned the existence of
> ServerContextFactory.get() with no arguments -- something I hadn't known
> about -- and I wanted to see how it worked.
>
> --tim
>
>

--
View this message in context: http://www.nabble.com/concurrency-issue-with-Factory--tp19243768p19338068.html

Sent from the DWR - Dev mailing list archive at Nabble.com.


---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@...
For additional commands, e-mail: dev-help@...

[tpeierls]

Checked in on trunk.

--tim

Joe Walker-3 wrote:

That's fine, Tim - go for it.
Joe.

On Fri, Sep 5, 2008 at 8:36 PM, tpeierls <[e-mail]> wrote:

>
> I see that Joe addressed this by making the builder field volatile.
>
> There's a slight improvement that could be made, however. As the code
> stands, the builder field is accessed twice in each get method, once to
> check for nullity and then again to use the non-null value.
>
> It would better to read the volatile field once into a temporary variable.
> It's more efficient and it prevents the (extremely unlikely) case where the
> value is set to null after being seen as non-null.
>
> I have the patch ready to check in, if you like, but it's just adding this
> line (in two methods) before the nullity test:
>
>    Builder<T> builder = this.builder;
>
> --tim
>