scala / bug

Scala 2 bug reports only. Please, no questions — proper bug reports only.
https://scala-lang.org
232 stars 21 forks source link

`ArrayBuilder` regression in scala `2.13.13`, `OutOfMemoryError` when adding empty arrays #12971

Closed alexdupre closed 7 months ago

alexdupre commented 8 months ago

Reproduction steps

Scala version: 2.13.13

The issue is reproducible only on scala 2.13.13, because a recent patch to the ArrayBuilder triggered it, but the incriminated code is probably the same in every scala release: https://github.com/scala/scala/blob/5aa3dc5ea77800ddd2a3bdf9e7feeea57484421c/src/library/scala/collection/mutable/ArrayBuffer.scala#L326-L331

As you can see from the code above, if the targetLen is 0 the array length is doubled.

Here you can find a minimal test-case: https://github.com/alexdupre/scala-2.13.13-bug

Just sbt run to reproduce it.

Problem

OutOfMemoryError because the underlying array is doubled at every iteration even if no elements have been added.

som-snytt commented 8 months ago

Thanks.

Iteration 23
Exception in thread "main" java.lang.OutOfMemoryError: Java heap space

Obviously it's because it hit 23 on Scala 2.

SethTisue commented 8 months ago

@scala/collections

OndrejSpanel commented 8 months ago

It is scary to see even heavily reviewed PR like #10448 can introduce bug like this.

som-snytt commented 8 months ago

@OndrejSpanel I'll comment here instead of the previous PR.

It's because the PR was heavily reviewed, not in spite of it.

In magic acts, it's called misdirection, but here it's just a form of distraction.

Also, sometimes a change benefits from a latency period, so that it can be viewed with fresh eyes. But you have to be motivated to return to a topic after six months or a year.

This is why we need more scalacheck and/or collections-laws.

som-snytt commented 8 months ago

I see I made that comment on the PR:

Put another way, I might say now, "What a stupid test, it should be a scalacheck, I can't even see what sort of boundary condition it thinks it is probing."

which sounds harsh out of context.