ListBuffer failure with insert, insertAll, update and updated; data corruption with insert, remove

[Simon Ochsenreither]

While triaging issues SI-6632 and SI-6633 I looked for similar code patterns and found this highly questionable behavior in ListBuffer#remove, which has an off-by-one error and data corruption issues: SI-6634.

Imho we should remove this undocumented (and unsuccessful) bounds-fixing behaviour and replace it with appropriate bound checks.

Opinions?

[Josh Suereth]

I'm all for it.  Do you know when the bounds checks were added and for what purpose?

[Daniel Sobral]

Git blame can answer that. I looked at it, but I'm not sure which lines Simon is speaking of. A lot of remove is either from the original new collections checking or an uncommented 2005 commit.

--
Daniel C. Sobral

I travel to the future all the time.

[Josh Suereth]

Enlightening :-)

So, perhaps then we can just fix things for 2.10.1 to match the docs.

This is where "binary compatibility" policies become fun.   We could still break users who expect negative indices to work because it did before.

I think our only policy going forward is to not break documented behavior vs. folks relying on implementation details.

Let's try to fix this for 2.10.1.   Deadline is Dec 31st before that goes into lockdown.

[Daniel Sobral]

On Fri, Nov 9, 2012 at 10:38 AM, Josh Suereth <joshua....@gmail.com> wrote:

Enlightening :-)

So, perhaps then we can just fix things for 2.10.1 to match the docs.

This is where "binary compatibility" policies become fun.   We could still break users who expect negative indices to work because it did before.

I think our only policy going forward is to not break documented behavior vs. folks relying on implementation details.

Let's try to fix this for 2.10.1.   Deadline is Dec 31st before that goes into lockdown.

May I suggest documenting current behavior with negative indices to be broken on 2.10.0? I'm assuming purely documenting commits are allowed, though, of course, I might be wrong.

 

On Nov 9, 2012 7:27 AM, "Daniel Sobral" <dcso...@gmail.com> wrote:

Git blame can answer that. I looked at it, but I'm not sure which lines Simon is speaking of. A lot of remove is either from the original new collections checking or an uncommented 2005 commit.

On Fri, Nov 9, 2012 at 4:52 AM, Josh Suereth <joshua....@gmail.com> wrote:

I'm all for it.  Do you know when the bounds checks were added and for what purpose?

On Nov 8, 2012 10:51 PM, "Simon Ochsenreither" <simon.och...@gmail.com> wrote:

While triaging issues SI-6632 and SI-6633 I looked for similar code patterns and found this highly questionable behavior in ListBuffer#remove, which has an off-by-one error and data corruption issues: SI-6634.

Imho we should remove this undocumented (and unsuccessful) bounds-fixing behaviour and replace it with appropriate bound checks.

Opinions?

--
Daniel C. Sobral

I travel to the future all the time.

[Daniel Sobral]

Otherwise, it could go into a "known bugs" list on the release notes.

[Josh Suereth]

If there's an RC3, then such a commit is ok, although we like to avoid anything unrelated to blocking issues..   In absense of a blocker, no commits go in, RC2 becomes final.

[Simon Ochsenreither]

Was it intended that two of those bugs ended up in the RC2 announcement?

Btw, if we are in the state where RC's could probably ship as final, we should write that down more explicitly. (Along the lines of “if we find no additional blocker, RCx will become the final release”.) This makes people test their code which would normally not participate in earlier testing phases.

[Lukas Rytz]

On Fri, Nov 9, 2012 at 4:30 PM, Simon Ochsenreither <simon.och...@gmail.com> wrote:

Was it intended that two of those bugs ended up in the RC2 announcement?

Btw, if we are in the state where RC's could probably ship as final, we should write that down more explicitly. (Along the lines of “if we find no additional blocker, RCx will become the final release”.)

Every RC is like that, RCs can only go out if they "could probably ship as final".

[Josh Suereth]

Yes.  Any issue marked critical shows in release notes.   Blockers, by nature of "blocking" must be fixed.  Critical are ones you should be aware of.
AND EVERY RC is a potential release.

That's why we avoid changing much in 2.10.0 because it should be good enough to release now.

[Paul Phillips]

On Fri, Nov 9, 2012 at 8:41 AM, Lukas Rytz <lukas...@epfl.ch> wrote:

Every RC is like that, RCs can only go out if they "could probably ship as final".

I'm guessing he's saying that it hasn't always been like that, and it would have been a good idea to make it abundantly clear that we're (apparently) in a new era of meaning it. We're not exactly dripping with credibility that anything we call a "release candidate" is a "candidate for release." I'm sorry that the only history we have of 2.8 is destroyed by svnmerge, because I'm sure git could paint a detailed picture of how ready-to-ship 2.8.0-RC1 was.

[Simon Ochsenreither]

I'm guessing he's saying that it hasn't always been like that, and it would have been a good idea to make it abundantly clear that we're (apparently) in a new era of meaning it. We're not exactly dripping with credibility that anything we call a "release candidate" is a "candidate for release." I'm sorry that the only history we have of 2.8 is destroyed by svnmerge, because I'm sure git could paint a detailed picture of how ready-to-ship 2.8.0-RC1 was.

Thanks, Paul, you said it better than I could have. :-)

Just to describe the experience from an external, non-EPFL/non-Typesafe person's view: both the switch to "bugfix-only" mode and the scarcity of RCs were a huge surprise to me. Based on earlier release series (2.7.x, 2.8.x, and partly 2.9.x I expected twice or three times as much release candidates).

Imho, there was not enough time between telling people "if you want stuff merged, do a pr before the deadline" and deadline itself. Maybe this can be improved in the next release series.

[Simon Ochsenreither]

Yes.  Any issue marked critical shows in release notes.   Blockers, by nature of "blocking" must be fixed.  Critical are ones you should be aware of.
AND EVERY RC is a potential release.

Ah, right. Thanks! Feel free to change the priority of SI-6632, SI-6633 and SI-6634 to the appropriate level then.

[Josh Suereth]

So 2.10.1 goes into lockdown on Dec. 31st.  Consider yourself warned.

In the future dates will be fixed, and things should be more open.   2.10.0 was an odd duck in that it saw changes in management/ownership/expectations as it developed.

This (few RCs, hard restrictions on patches) is the new normal., and Long overdue.

[Adriaan Moors]

We're tracking the release schedule in the following public calendar:

https://www.google.com/calendar/embed?src=typesafe.com_i4ri6i...@group.calendar.google.com

(There may be some inaccuracies in there right now. We'll communicate any changes to scala-internals.)

[Simon Ochsenreither]

Cool, thanks!

Are there btw plans to cut a 2.9.3 release?

[Josh Suereth]

There's talk, but no formal plans yet.