Fixed
Details
Assignee
Michael OffnerMichael OffnerReporter
Bruce KirkpatrickBruce KirkpatrickPriority
CriticalLabels
Fix versions
New Issue warning screen
Before you create a new Issue, please post to the mailing list first https://dev.lucee.org
Once the issue has been verified, one of the Lucee team will ask you to file an issue
Sprint
NoneAffects versions
Details
Details
Assignee
Michael Offner
Michael OffnerReporter
Bruce Kirkpatrick
Bruce KirkpatrickPriority
Labels
Fix versions
New Issue warning screen
Before you create a new Issue, please post to the mailing list first https://dev.lucee.org
Once the issue has been verified, one of the Lucee team will ask you to file an issue
Sprint
None
Affects versions
Created 25 November 2018 at 22:54
Updated 27 March 2024 at 08:46
Resolved 12 July 2019 at 15:38
The documentation claims the ArrayNew() synchronized option is defaulting to true, but it isn't anymore. Arrays also historically used to be synchronized in Lucee 4.5 and maybe prior versions too.
https://docs.lucee.org/reference/functions/arraynew.html
I looked at the implementation, and I see ArrayList is used by default with no synchronization as the underlying data type. Only arrays with a larger dimension are synchronized in ArrayClassic, which used to be the only kind of Array.
Also, are you sure you want the new type argument to be in between dimension and synchronized? Everyone using that on ACF and coming to Lucee would have to rewrite those manually to get compatible. Easier to make that argument #3 instead. The documentation for this brand new feature is currently missing as well.
It is probably better to make synchronization the default so a developer chooses when to incur this risk since thread safety bugs are so confusing and random when they come up.
I noticed this because ArraySort is able to throw concurrentModificationException in my real application. I duplicated the array to fix that. If the CFML developer was being told to synchronize arrays themselves, the documentation should be updated to warn them the default was changed in X version.
I see Vector commented out. I read about Vector, which is synchronized unlike ArrayList. It sounds like Vector is discouraged because individually locking every array operation is inefficient compared to 0 or 1 lock around the group of operations. It seems like you have Lucee setup the fastest performing way now, but would need to announce the behavior change to get people ready for it. Maybe the Lucee admin option route is an easier way to handle it globally since those seeking performance, could be willing to change the default behavior. This is a break in compatibility with both ACF and older Lucee so it is currently behaving like a bug.
Because ArrayImpl.java is used for lots of internal objects that don't seem to require thread safety it seems like ArrayImpl.java should stay the same. A new class is better, or we do nothing and let people fix their own Concurrency errors individually. It will give the impression Lucee is not stable if one brought in code from ACF that was thread-safe there, and now isn't. It would protect Lucee reputation to give the Lucee admin treatment to this idea, instead of receiving more forum/bug reports on it.
To make it easier for developers to understand thread safety issues, maybe it would help if you augmented ConcurrentModificationException error reporting with a better description of the problem when you can detect it happened on a CFML object. A better error could say "Write operations to this type of array are not thread-safe, and a ConcurrentModificationException was thrown, meaning another thread attempt to access the array while it was being modified. You must create a synchronized array (see ArrayNew documentation), add a lock around the array or create a duplicate() of the array to avoid this error." and maybe show the array variable name with reflection.
Looks like a quick fix in ArrayUtil.java getInstance() if we wanted to change it back or make an option for it.