You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@maven.apache.org by Michael Osipov <mi...@apache.org> on 2017/05/25 19:12:49 UTC
[MNG-6164] Collections inconsistently immutable
Who seconds MNG-6164 for 3.5.1?
This is a non-functional consistency fix. All ITs pass.
Michael
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@maven.apache.org
For additional commands, e-mail: dev-help@maven.apache.org
Re: [MNG-6164] Collections inconsistently immutable
Posted by Tibor Digana <ti...@googlemail.com>.
Using immutable collections in Maven was exactly my advice in 2013/2014.
Usually it is bad to share collections especially when using concurrent
Threads.
Normally there should be service methods which change some object state
*safely* or thread safe. Personally I think plugins will break with
immutable collections, but what is better:
1. to be open and open for happening unsafe state or
2. being more conservative which requires more effort with implementing
service methods.
On Thu, May 25, 2017 at 9:12 PM, Michael Osipov <mi...@apache.org> wrote:
> Who seconds MNG-6164 for 3.5.1?
>
> This is a non-functional consistency fix. All ITs pass.
>
> Michael
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: dev-unsubscribe@maven.apache.org
> For additional commands, e-mail: dev-help@maven.apache.org
>
>
--
Cheers
Tibor
Re: [MNG-6164] Collections inconsistently immutable
Posted by Robert Scholte <rf...@apache.org>.
+1
Robert
On Sun, 28 May 2017 12:01:20 +0200, Michael Osipov <mi...@apache.org>
wrote:
> Am 2017-05-25 um 21:12 schrieb Michael Osipov:
>> Who seconds MNG-6164 for 3.5.1?
>>
>> This is a non-functional consistency fix. All ITs pass.
>
> Anyone?
>
>
> ---------------------------------------------------------------------
> 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: [MNG-6164] Collections inconsistently immutable
Posted by Hervé BOUTEMY <he...@free.fr>.
I started to review each modification in depth and write a summary, but I
stopped since I know nobody will read it because of TLDR;
I preferred rewrite the issue description as:
"All those methods should return a collection with consistent "mutability":
either mutable, either immutable.
Given empty immutable collections do not cause harm until now, switching
consistently to immutable collections is more conservative and should not be
risky"
I think it would be useful to add the info on immutable collection in API
javadoc, since some classes are not consistent on every returned collection
(for example DefaultArtifact, where getDependencyTrail() and
getAvailableVersions() stay mutable)
From what I saw, the change does not really give the documented API
consistency that one could expect, but should not make real harm (I hope).
And I buy your "Changing the returned list [...] should be an easy fix by
plugin maintainers"
I won't second this change but I won't block it
Regards,
Hervé
Le dimanche 28 mai 2017, 17:01:21 CEST Robert Scholte a écrit :
> I think we should do this step, either now or with a next major release.
>
> And it's about the *returned* Lists which now become unmodifiable. As far
> as I can see all these changes have a setter which should be used to
> specify a new list.
> These changes should protect developers which use the returned list and
> manipulate it for own business logic (e.g. remove unwanted entries). But
> this should not change the original state, unless they called the matching
> setter.
> Changing the returned list should be considered a bad practice and should
> be an easy fix by plugin maintainers.
>
> Robert
>
> On Sun, 28 May 2017 13:01:41 +0200, Hervé BOUTEMY <he...@free.fr>
>
> wrote:
> > I'm not confident on this one.
> >
> > I understand the inconsistency issue, that may lead to edge-case issues
> > in
> > plugins or extensions (that unexpectedly get an immutable empty list
> > when they
> > usually get a modifiable non-empty list)
> >
> > Is it the right solution to change every non-empty list to immutable?
> > Why wouldn't be the solution to make every returned empty list mutable?
> > Or a mix: on some cases, immutable is a good feature, but on others
> > consistent
> > mutable would be the good choice?
> >
> > Do you know how many plugins or extensions this update will break?
> > I know that consistent breaks are better than inconsistent breaks (on
> > empty
> > lists), but is it the right time to take such risk?
> >
> > I know that core ITs pass: that does not mean that external plugins won't
> > break (thousands in Central only, I don't know how many corporate plugins
> > exist)
> >
> >
> > How many cases are changed? In which areas of Maven API?
> > That is the information I need to vote for this change, knowing the
> > effective
> > impact it will have
> >
> > Regards,
> >
> > Hervé
> >
> > https://issues.apache.org/jira/browse/MNG-6164
> >
> > Le dimanche 28 mai 2017, 12:01:20 CEST Michael Osipov a écrit :
> >> Am 2017-05-25 um 21:12 schrieb Michael Osipov:
> >> > Who seconds MNG-6164 for 3.5.1?
> >> >
> >> > This is a non-functional consistency fix. All ITs pass.
> >>
> >> Anyone?
> >>
> >>
> >> ---------------------------------------------------------------------
> >> 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
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@maven.apache.org
For additional commands, e-mail: dev-help@maven.apache.org
Re: [MNG-6164] Collections inconsistently immutable
Posted by Michael Osipov <mi...@apache.org>.
Am 2017-05-28 um 20:55 schrieb Hervé BOUTEMY:
> Le dimanche 28 mai 2017, 19:50:03 CEST Michael Osipov a écrit :
>>> How many cases are changed? In which areas of Maven API?
>>> That is the information I need to vote for this change, knowing the
>>> effective impact it will have
>>
>> As far as I can see for the changes, they all look like read-only
>> structures.
> yes, that's my conclusion after having looked at many cases, but I confess I
> ddin't really dig into each case
>
>>
>> I fully understand your concerns. Let me run plugins ITs and see return,
>> we can postpone to 3.6 if we think this is too problematic.
> yes, let's consider Maven plugins are a good overview of what can happen
> If you can do this, this will lower unexpected risks: thank you
Hervé,
since plugin ITs are not affected, are we good to merge this into master?
Michael
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@maven.apache.org
For additional commands, e-mail: dev-help@maven.apache.org
Re: [MNG-6164] Collections inconsistently immutable
Posted by Hervé BOUTEMY <he...@free.fr>.
Le dimanche 28 mai 2017, 19:50:03 CEST Michael Osipov a écrit :
> > How many cases are changed? In which areas of Maven API?
> > That is the information I need to vote for this change, knowing the
> > effective impact it will have
>
> As far as I can see for the changes, they all look like read-only
> structures.
yes, that's my conclusion after having looked at many cases, but I confess I
ddin't really dig into each case
>
> I fully understand your concerns. Let me run plugins ITs and see return,
> we can postpone to 3.6 if we think this is too problematic.
yes, let's consider Maven plugins are a good overview of what can happen
If you can do this, this will lower unexpected risks: thank you
Regards,
Hervé
>
> Michael
>
> > Le dimanche 28 mai 2017, 12:01:20 CEST Michael Osipov a écrit :
> >> Am 2017-05-25 um 21:12 schrieb Michael Osipov:
> >>> Who seconds MNG-6164 for 3.5.1?
> >>>
> >>> This is a non-functional consistency fix. All ITs pass.
> >>
> >> Anyone?
> >>
> >>
> >> ---------------------------------------------------------------------
> >> 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
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@maven.apache.org
For additional commands, e-mail: dev-help@maven.apache.org
Re: [MNG-6164] Collections inconsistently immutable
Posted by Robert Scholte <rf...@apache.org>.
I think we should do this step, either now or with a next major release.
And it's about the *returned* Lists which now become unmodifiable. As far
as I can see all these changes have a setter which should be used to
specify a new list.
These changes should protect developers which use the returned list and
manipulate it for own business logic (e.g. remove unwanted entries). But
this should not change the original state, unless they called the matching
setter.
Changing the returned list should be considered a bad practice and should
be an easy fix by plugin maintainers.
Robert
On Sun, 28 May 2017 13:01:41 +0200, Hervé BOUTEMY <he...@free.fr>
wrote:
> I'm not confident on this one.
>
> I understand the inconsistency issue, that may lead to edge-case issues
> in
> plugins or extensions (that unexpectedly get an immutable empty list
> when they
> usually get a modifiable non-empty list)
>
> Is it the right solution to change every non-empty list to immutable?
> Why wouldn't be the solution to make every returned empty list mutable?
> Or a mix: on some cases, immutable is a good feature, but on others
> consistent
> mutable would be the good choice?
>
> Do you know how many plugins or extensions this update will break?
> I know that consistent breaks are better than inconsistent breaks (on
> empty
> lists), but is it the right time to take such risk?
>
> I know that core ITs pass: that does not mean that external plugins won't
> break (thousands in Central only, I don't know how many corporate plugins
> exist)
>
>
> How many cases are changed? In which areas of Maven API?
> That is the information I need to vote for this change, knowing the
> effective
> impact it will have
>
> Regards,
>
> Hervé
>
> https://issues.apache.org/jira/browse/MNG-6164
>
> Le dimanche 28 mai 2017, 12:01:20 CEST Michael Osipov a écrit :
>> Am 2017-05-25 um 21:12 schrieb Michael Osipov:
>> > Who seconds MNG-6164 for 3.5.1?
>> >
>> > This is a non-functional consistency fix. All ITs pass.
>>
>> Anyone?
>>
>>
>> ---------------------------------------------------------------------
>> 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: [MNG-6164] Collections inconsistently immutable
Posted by Michael Osipov <mi...@apache.org>.
Am 2017-05-28 um 13:01 schrieb Hervé BOUTEMY:
> I'm not confident on this one.
>
> I understand the inconsistency issue, that may lead to edge-case issues in
> plugins or extensions (that unexpectedly get an immutable empty list when they
> usually get a modifiable non-empty list)
>
> Is it the right solution to change every non-empty list to immutable?
Neither is unless you exactly know how the lists will be used.
> Why wouldn't be the solution to make every returned empty list mutable?
This is also I am not in favor of neither one, but it has to be
predictable, thus consisitent.
> Or a mix: on some cases, immutable is a good feature, but on others consistent
> mutable would be the good choice?
The caller does not know wether he will receive a populated or empty list.
> Do you know how many plugins or extensions this update will break?
> I know that consistent breaks are better than inconsistent breaks (on empty
> lists), but is it the right time to take such risk?
I can run all plugin ITs from our aggregator and see how they work.
> I know that core ITs pass: that does not mean that external plugins won't
> break (thousands in Central only, I don't know how many corporate plugins
> exist)
>
>
> How many cases are changed? In which areas of Maven API?
> That is the information I need to vote for this change, knowing the effective
> impact it will have
As far as I can see for the changes, they all look like read-only
structures.
I fully understand your concerns. Let me run plugins ITs and see return,
we can postpone to 3.6 if we think this is too problematic.
Michael
> Le dimanche 28 mai 2017, 12:01:20 CEST Michael Osipov a écrit :
>> Am 2017-05-25 um 21:12 schrieb Michael Osipov:
>>> Who seconds MNG-6164 for 3.5.1?
>>>
>>> This is a non-functional consistency fix. All ITs pass.
>>
>> Anyone?
>>
>>
>> ---------------------------------------------------------------------
>> 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: [MNG-6164] Collections inconsistently immutable
Posted by Hervé BOUTEMY <he...@free.fr>.
I'm not confident on this one.
I understand the inconsistency issue, that may lead to edge-case issues in
plugins or extensions (that unexpectedly get an immutable empty list when they
usually get a modifiable non-empty list)
Is it the right solution to change every non-empty list to immutable?
Why wouldn't be the solution to make every returned empty list mutable?
Or a mix: on some cases, immutable is a good feature, but on others consistent
mutable would be the good choice?
Do you know how many plugins or extensions this update will break?
I know that consistent breaks are better than inconsistent breaks (on empty
lists), but is it the right time to take such risk?
I know that core ITs pass: that does not mean that external plugins won't
break (thousands in Central only, I don't know how many corporate plugins
exist)
How many cases are changed? In which areas of Maven API?
That is the information I need to vote for this change, knowing the effective
impact it will have
Regards,
Hervé
https://issues.apache.org/jira/browse/MNG-6164
Le dimanche 28 mai 2017, 12:01:20 CEST Michael Osipov a écrit :
> Am 2017-05-25 um 21:12 schrieb Michael Osipov:
> > Who seconds MNG-6164 for 3.5.1?
> >
> > This is a non-functional consistency fix. All ITs pass.
>
> Anyone?
>
>
> ---------------------------------------------------------------------
> 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: [MNG-6164] Collections inconsistently immutable
Posted by Michael Osipov <mi...@apache.org>.
Am 2017-05-25 um 21:12 schrieb Michael Osipov:
> Who seconds MNG-6164 for 3.5.1?
>
> This is a non-functional consistency fix. All ITs pass.
Anyone?
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@maven.apache.org
For additional commands, e-mail: dev-help@maven.apache.org