Fix ConcurrentModificationException in TestMethodController

[lkishalmi]

Just happened to bump into this CME, while working on Jenkinsfile support. It seems this has been reported as #5148.

It seems the CME happens as the methods list is a live one passed over as parameter, so the collection can be changed during the time of methods.forEach(tm -> removed.remove(tm)). The solution would be to copy that collection before do the remove work. Interestingly we do have a copy of right after that call, so a simple reorder would solve the issue. I've just replaced the forEach call with keySet().removeAll as I think that reads better.

Otherwise the change is trivial. If it does not make into NB20, then I'd rebase this for master.

[mbien]

the root of this issue is likely that modifiable collections are shared over the document properties.

I believe the risk of CME is still there, it just moved from the forEach method to the copy constructor of the HashSet/HashMap? It should still fail if the concurrent modification is noticed while it is iterating to build the copy. (CMEs are not guaranteed to be thrown in many cases, its more a best-effort manner for bug detection, it often simply compares col sizes)

edit: i haven't checked the access pattern yet, I don't know from where else the collections are accessed, one red flag is that the empty collection is added to the properties and modified afterwards, i suppose someone could listen on property changes and react while the method is still running.

edit2: usages of the keys are:

• https://github.com/search?q=repo%3Aapache%2Fnetbeans+DOCUMENT_METHODS_KEY&type=code
• https://github.com/search?q=repo%3Aapache%2Fnetbeans+DOCUMENT_ANNOTATION_LINES_KEY&type=code
• https://github.com/search?q=repo%3Aapache%2Fnetbeans+DOCUMENT_ANNOTATIONS_KEY&type=code

[mbien]

possible way out is the copy on write pattern:

for write:

• defensive copy after props.get
• modify defensive copy
• put update with Collecitons.unmodifiableFoo(defensiveCopy) into props

for read:

• props.get and read remains simple

or synchronized collections, but the client has to know that it is synchonized and use the lock. JDK has also copy-on-write lists and sets but no map I believe.

edit: lock-free copy-on-write works only reliably if there is only one writer, otherwise writes can be reset by concurrent writers.

[lkishalmi]

@mbien Thanks for the review! It made me look what's upper on the road and found the one and only call to this method in ComputeTestMethodsImpl and indeed the CME has been built in the system. Here I've just added a defensive copy, this time I hope in the right place. Probably an extra unmodifiable wrapper would have been nice, though this late in the release phase I do not wish extra exceptions popping up due to that.

[mbien]

this might work, thanks!

We should revisit this for NB21 and either make the shared collections immutable, or find some other simple way of access control.
(at the very least it has to be documented in-code so that nobody optimizes the defensive copies away again :))

[neilcsmith-net]

👍

We should revisit this for NB21 and either make the shared collections immutable

*.copyOf(..) ftw!

[mbien]

I always forget that the copyOf methods exist. I should use them more.

[neilcsmith-net]

They have the added benefit of being a no-op in cases where you stack defensive copies.