You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@maven.apache.org by Kristian Rosenvold <kr...@gmail.com> on 2013/01/09 20:43:11 UTC
Wagons and thread safety....
I did a little investigation into the wagon code, wanting to
investigate some suspicions I've had for some time.
It seems to me like a single wagon instance is definitely not thread
safe, which is by itself ok. But the wagon instances seem
to be re-used by different threads, which would seem to indicate
serial thread confinement. But looking at some of the
code in the wagon classes has me deeply suspicious to the correctness
of such a confinement.
I'm quite certain the instance needs to be confined to a single thread
for the duration of a connection. Is this the case ?
It looks to me like re-using a wagon instance in a different thread is
a /very/ risky business due to the
stateful nature of the wagons; I'm not even sure it can be done
correctly. But it seems to be done, is this
a concious decision that is actually verified in the code ?
I must hasten to add that I am quite unfamiliar with the code, my
analysis is just based on reading some code
and doing some simple instrumentations.
Kristian
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@maven.apache.org
For additional commands, e-mail: dev-help@maven.apache.org
Re: Wagons and thread safety....
Posted by Jörg Schaible <Jo...@scalaris.com>.
Stuart McCulloch wrote:
> On 10 Jan 2013, at 13:19, Kristian Rosenvold wrote:
[snip]
>> In one way kind of neat; since the statement has "both"
>> @plexus.requirement and final it's fairly obvious who sets it;
>> although the semantics are definitely not java101 ;) Do you know if
>> this works with old plexus too ? (2.2.1)
>
> It should also work with old Plexus, as that also uses setAccessible to
> break encapsulation - I seem to remember seeing some Plexus requirements
> that were already marked final
Requires at least Java 5 runtime i.e. fine with M221.
Cheers,
Jörg
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@maven.apache.org
For additional commands, e-mail: dev-help@maven.apache.org
Re: Wagons and thread safety....
Posted by Stuart McCulloch <mc...@gmail.com>.
On 10 Jan 2013, at 13:19, Kristian Rosenvold wrote:
> 2013/1/10 Stuart McCulloch <mc...@gmail.com>:
>> There's no guarantee that Plexus field injection will be synchronized, so if you're constructing an object in one thread and immediately handing it off to another >thread without any intervening synchronization then you should use volatile fields or ideally introduce a synchronized setter+getter in the outermost object. But even >without injection you'd want to do this if you were setting values in one thread and wanting another thread to see them.
>
> Ok, this means all the @plexus.requirement fields in wagon should be
> volatile or similar.
>
>> One benefit of JSR330 is that you can use constructor injection along with final fields which avoids this kind of thread-visibility issue, since they are frozen and made > visible to other threads when the constructor completes.
>
> Constructor injection is king; too bad we can't have that and maven
> 2.2.1 compatibility...?
Not unless we migrate maven2 over to the new container - but you could always introduce some sort of factory/clone step into the wagon code that constructed a new object for the other thread from the injected fields.
>> PS. you can actually mark a private field as final and still have it injected, because the reflection code uses "setAccessible" to break through the encapsulation and >this also overrides the 'final-ness' as described in http://jeremymanson.blogspot.co.uk/2008/07/immutability-in-java-part-3.html while still preserving the thread->visibility effect of final - but this is not recommended as it obscures the actual intent.
>
> In one way kind of neat; since the statement has "both"
> @plexus.requirement and final it's fairly obvious who sets it;
> although the semantics are definitely not java101 ;) Do you know if
> this works with old plexus too ? (2.2.1)
It should also work with old Plexus, as that also uses setAccessible to break encapsulation - I seem to remember seeing some Plexus requirements that were already marked final
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@maven.apache.org
For additional commands, e-mail: dev-help@maven.apache.org
Re: Wagons and thread safety....
Posted by Kristian Rosenvold <kr...@gmail.com>.
2013/1/10 Stuart McCulloch <mc...@gmail.com>:
> There's no guarantee that Plexus field injection will be synchronized, so if you're constructing an object in one thread and immediately handing it off to another >thread without any intervening synchronization then you should use volatile fields or ideally introduce a synchronized setter+getter in the outermost object. But even >without injection you'd want to do this if you were setting values in one thread and wanting another thread to see them.
Ok, this means all the @plexus.requirement fields in wagon should be
volatile or similar.
> One benefit of JSR330 is that you can use constructor injection along with final fields which avoids this kind of thread-visibility issue, since they are frozen and made > visible to other threads when the constructor completes.
Constructor injection is king; too bad we can't have that and maven
2.2.1 compatibility...?
> PS. you can actually mark a private field as final and still have it injected, because the reflection code uses "setAccessible" to break through the encapsulation and >this also overrides the 'final-ness' as described in http://jeremymanson.blogspot.co.uk/2008/07/immutability-in-java-part-3.html while still preserving the thread->visibility effect of final - but this is not recommended as it obscures the actual intent.
In one way kind of neat; since the statement has "both"
@plexus.requirement and final it's fairly obvious who sets it;
although the semantics are definitely not java101 ;) Do you know if
this works with old plexus too ? (2.2.1)
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@maven.apache.org
For additional commands, e-mail: dev-help@maven.apache.org
Re: Wagons and thread safety....
Posted by Stuart McCulloch <mc...@gmail.com>.
On 10 Jan 2013, at 07:57, Kristian Rosenvold wrote:
> It seems like I made some kind of mistake in my initial analysis, so
> it looks better !
>
> Used from core, the wagons are consistently created on one thread and
> handed off to another. From the handoff they seem to be confined to
> that thread, which should work.
>
> Now there is the issue of the @plexus.requirement annotated fields;
> which are wired by plexus (like ScmManager in ScmWagon). Does anyone
> know if the wiring process in plexus is synchronized ?
There's no guarantee that Plexus field injection will be synchronized, so if you're constructing an object in one thread and immediately handing it off to another thread without any intervening synchronization then you should use volatile fields or ideally introduce a synchronized setter+getter in the outermost object. But even without injection you'd want to do this if you were setting values in one thread and wanting another thread to see them.
One benefit of JSR330 is that you can use constructor injection along with final fields which avoids this kind of thread-visibility issue, since they are frozen and made visible to other threads when the constructor completes.
PS. you can actually mark a private field as final and still have it injected, because the reflection code uses "setAccessible" to break through the encapsulation and this also overrides the 'final-ness' as described in http://jeremymanson.blogspot.co.uk/2008/07/immutability-in-java-part-3.html while still preserving the thread-visibility effect of final - but this is not recommended as it obscures the actual intent.
> If not, I should think all the "plexus.requirement" fields in scm really need to
> be volatile, since they are populated post-construct and immediately
> handed off to another thread.
>
> Kristian
>
>
> 2013/1/9 Olivier Lamy <ol...@apache.org>:
>> Wagon are noted "per-lookup".
>> Have a look at DefaultWagonManager in core, this one always do a
>> container.lookup to get a new fresh wagon instance.
>> So normally plugins using WagonManager must not have any issues
>> (except reusing the same wagon instance)
>>
>> 2013/1/9 Kristian Rosenvold <kr...@gmail.com>:
>>> To illustrate the point I'm trying to make:
>>>
>>> org.apache.maven.wagon.shared.http4.AbstractHttpClientWagon line 240:
>>>
>>> protected ClientConnectionManager clientConnectionManager = new
>>> SingleClientConnManager();
>>>
>>> Now read the docs for SingleClientConnManager (annotated as @ThreadSafe!):
>>>
>>> "Even though this class is thread-safe it ought to be used by one
>>> execution thread only"
>>>
>>> This "ought to" means that it's probably unwise to handoff such an
>>> instance to a different thread down the line, which we do. So it does
>>> not support serial thread confinement. It makes me wonder about the
>>> @ThreadSafe annotation....
>>>
>>> Kristian
>>>
>>> ---------------------------------------------------------------------
>>> To unsubscribe, e-mail: dev-unsubscribe@maven.apache.org
>>> For additional commands, e-mail: dev-help@maven.apache.org
>>>
>>
>>
>>
>> --
>> Olivier Lamy
>> Talend: http://coders.talend.com
>> http://twitter.com/olamy | http://linkedin.com/in/olamy
>>
>> ---------------------------------------------------------------------
>> To unsubscribe, e-mail: dev-unsubscribe@maven.apache.org
>> For additional commands, e-mail: dev-help@maven.apache.org
>>
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: dev-unsubscribe@maven.apache.org
> For additional commands, e-mail: dev-help@maven.apache.org
>
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@maven.apache.org
For additional commands, e-mail: dev-help@maven.apache.org
Re: Wagons and thread safety....
Posted by Kristian Rosenvold <kr...@gmail.com>.
It seems like I made some kind of mistake in my initial analysis, so
it looks better !
Used from core, the wagons are consistently created on one thread and
handed off to another. From the handoff they seem to be confined to
that thread, which should work.
Now there is the issue of the @plexus.requirement annotated fields;
which are wired by plexus (like ScmManager in ScmWagon). Does anyone
know if the wiring process in plexus is synchronized ? If not, I
should think all the "plexus.requirement" fields in scm really need to
be volatile, since they are populated post-construct and immediately
handed off to another thread.
Kristian
2013/1/9 Olivier Lamy <ol...@apache.org>:
> Wagon are noted "per-lookup".
> Have a look at DefaultWagonManager in core, this one always do a
> container.lookup to get a new fresh wagon instance.
> So normally plugins using WagonManager must not have any issues
> (except reusing the same wagon instance)
>
> 2013/1/9 Kristian Rosenvold <kr...@gmail.com>:
>> To illustrate the point I'm trying to make:
>>
>> org.apache.maven.wagon.shared.http4.AbstractHttpClientWagon line 240:
>>
>> protected ClientConnectionManager clientConnectionManager = new
>> SingleClientConnManager();
>>
>> Now read the docs for SingleClientConnManager (annotated as @ThreadSafe!):
>>
>> "Even though this class is thread-safe it ought to be used by one
>> execution thread only"
>>
>> This "ought to" means that it's probably unwise to handoff such an
>> instance to a different thread down the line, which we do. So it does
>> not support serial thread confinement. It makes me wonder about the
>> @ThreadSafe annotation....
>>
>> Kristian
>>
>> ---------------------------------------------------------------------
>> To unsubscribe, e-mail: dev-unsubscribe@maven.apache.org
>> For additional commands, e-mail: dev-help@maven.apache.org
>>
>
>
>
> --
> Olivier Lamy
> Talend: http://coders.talend.com
> http://twitter.com/olamy | http://linkedin.com/in/olamy
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: dev-unsubscribe@maven.apache.org
> For additional commands, e-mail: dev-help@maven.apache.org
>
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@maven.apache.org
For additional commands, e-mail: dev-help@maven.apache.org
Re: Wagons and thread safety....
Posted by Olivier Lamy <ol...@apache.org>.
Wagon are noted "per-lookup".
Have a look at DefaultWagonManager in core, this one always do a
container.lookup to get a new fresh wagon instance.
So normally plugins using WagonManager must not have any issues
(except reusing the same wagon instance)
2013/1/9 Kristian Rosenvold <kr...@gmail.com>:
> To illustrate the point I'm trying to make:
>
> org.apache.maven.wagon.shared.http4.AbstractHttpClientWagon line 240:
>
> protected ClientConnectionManager clientConnectionManager = new
> SingleClientConnManager();
>
> Now read the docs for SingleClientConnManager (annotated as @ThreadSafe!):
>
> "Even though this class is thread-safe it ought to be used by one
> execution thread only"
>
> This "ought to" means that it's probably unwise to handoff such an
> instance to a different thread down the line, which we do. So it does
> not support serial thread confinement. It makes me wonder about the
> @ThreadSafe annotation....
>
> Kristian
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: dev-unsubscribe@maven.apache.org
> For additional commands, e-mail: dev-help@maven.apache.org
>
--
Olivier Lamy
Talend: http://coders.talend.com
http://twitter.com/olamy | http://linkedin.com/in/olamy
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@maven.apache.org
For additional commands, e-mail: dev-help@maven.apache.org
Re: Wagons and thread safety....
Posted by Kristian Rosenvold <kr...@gmail.com>.
To illustrate the point I'm trying to make:
org.apache.maven.wagon.shared.http4.AbstractHttpClientWagon line 240:
protected ClientConnectionManager clientConnectionManager = new
SingleClientConnManager();
Now read the docs for SingleClientConnManager (annotated as @ThreadSafe!):
"Even though this class is thread-safe it ought to be used by one
execution thread only"
This "ought to" means that it's probably unwise to handoff such an
instance to a different thread down the line, which we do. So it does
not support serial thread confinement. It makes me wonder about the
@ThreadSafe annotation....
Kristian
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@maven.apache.org
For additional commands, e-mail: dev-help@maven.apache.org