ConcurrentModificationException in parallel build somewhere in p2 AbstractRepository #4709
Comments
|
As far as I have checked this is all properly synchronized. Nevertheless it does not protect from reentrant calls from the same thread. Also SO I think one must carefully review all call sites if they probably are problematic here and then fix in either P2 or Tycho. |
|
Note that the constructor copies the map:
And while this this is synchronized it does not copy the map: Probably it should clean the map and put all the new entries. It should not replace the OrderedMap I think. Probably this should be synchronized because it's the only method that access the properties field but isn't synchronized: |
|
This seems to be involved though not clear to me what is modifying the properties of the delegate:
|
I think it won't hurt to add an explicit |
|
I really don't like that in p2 the OrderedMap is replaced with some arbitrary map that can be modified elsewhere and thereby bypass the synchronization guards. I also think that getProperty not being synchronized is simply an oversight. If those were both fixed, I'm not sure anything would need to be done in Tycho. Or? |
It might be by intention... somewhere ... in any case I think its worth to try change that in P2 and see if any tests fail, if not merge it today and see if anything else fails during M3 testing.
I don't think it is that critical, but would not harm to do so
The only think is if we want to change something we should do it now so Tycho not needs to wait for just another release :-) |
- Make AbstractRepository.properties final. - Modify AbstractRepository.setProperties to do a clear and a putAll. - Make the only method that accesses AbstractRepository.properties without synchronization, AbstractRepository.getProperty(String), also be synchronized. eclipse-tycho/tycho#4709
- Make AbstractRepository.properties final. - Modify AbstractRepository.setProperties to do a clear and a putAll. - Make the only method that accesses AbstractRepository.properties without synchronization, AbstractRepository.getProperty(String), also be synchronized. eclipse-tycho/tycho#4709
Thanks, @merks!
Hmm, 2025-03 isn't that far away, so unless there's an obvious way to also synchronize inside Tycho layer, I think waiting for the new p2 is good enough. It's the first time I've seen this particular error in our build which runs almost daily. |
|
Yes this will hopefully be fixed with next eclipse release and I plan to do a new release timely after maven artifacts are published and integrated! |
I just got the following in a parallel build.
-T 6I am not sure whether this this is something that needs to be synchronized in the Tycho layer (or retried, similar to #3698?), or if this is an actual bug in P2, since
AbstractRepositoryManageralready uses asynchronized(repositoryLock) { ... }, and it still fails.The text was updated successfully, but these errors were encountered: