Skip to content

Commit 20d600f

Browse files
CopilotbedaHovorkaclaude
authored
Thread-safety: synchronize listener add/remove and snapshot in Dynamic* wrappers (#382)
* Initial plan * fix: synchronize listener add/remove and use snapshot in Dynamic* wrappers Co-authored-by: bedaHovorka <5263405+bedaHovorka@users.noreply.github.com> * fix: wrap long synchronized lines in DynamicRailSwitch.kt to pass detekt MaxLineLength Lines 182 and 199 were 122 chars (limit: 120). Split chained call onto next line. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * fix(review): add KDoc thread-safety notes, use named snapshot, add re-entrancy tests - Add @thread Safety Note KDoc to addPropertyChangeListener/removePropertyChangeListener in DynamicTrack, DynamicRailSemaphore, DynamicRailSwitch (matches BaseContext pattern) - DynamicRailSwitch: replace inline synchronized(...).forEach with named val snapshot for consistency with BaseContext and DynamicTrack (style alignment) - Add 2 re-entrancy tests per class (6 tests total): listener-added-during-callback and listener-removed-during-callback both verify no ConcurrentModificationException All 2000 tests pass. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * fix: use private lock object instead of synchronized(this) in Dynamic* wrappers Replace @synchronized (which uses synchronized(this)) with a dedicated private `listenersLock = Any()` field in DynamicTrack, DynamicRailSemaphore, and DynamicRailSwitch. All listener add/remove/snapshot operations now lock on the private object instead of the public instance reference. This fixes two SonarCloud issues: - Security Hotspot: "synchronized(this)" exposes the instance monitor to external callers (RSPEC-2082) - C Reliability Rating: using the object's own monitor for synchronization Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> --------- Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: bedaHovorka <5263405+bedaHovorka@users.noreply.github.com> Co-authored-by: Bedrich Hovorka <bedrich.hovorka@gmail.com> Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com>
1 parent e823c0b commit 20d600f

6 files changed

Lines changed: 176 additions & 20 deletions

File tree

‎src/main/kotlin/cz/vutbr/fit/interlockSim/objects/cells/DynamicRailSemaphore.kt‎

Lines changed: 11 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -46,6 +46,7 @@ sealed class DynamicRailSemaphore(
4646
* Listeners for signal state changes (Kotlin-native, no java.beans dependency).
4747
*/
4848
private val listeners = mutableListOf<ContextPropertyChangeListener>()
49+
private val listenersLock = Any()
4950

5051
// Static properties delegated from wrapped object
5152
// orientation and direction() are delegated from OrientedPathSeparator
@@ -73,7 +74,8 @@ sealed class DynamicRailSemaphore(
7374
// Fire property change event only if signal actually changed
7475
if (oldSignal != newSignal) {
7576
val evt = ContextChangeEvent("signal", oldSignal, newSignal)
76-
listeners.toList().forEach { it.propertyChange(evt) }
77+
val snapshot = synchronized(listenersLock) { listeners.toList() }
78+
snapshot.forEach { it.propertyChange(evt) }
7779
}
7880
}
7981

@@ -178,19 +180,25 @@ sealed class DynamicRailSemaphore(
178180
*
179181
* The listener will be notified when the "signal" property changes.
180182
*
183+
* **Thread Safety Note**: This method is synchronized to allow safe listener registration
184+
* from multiple threads, even though the simulation context itself is not thread-safe.
185+
*
181186
* @param listener The listener to add
182187
*/
183188
fun addPropertyChangeListener(listener: ContextPropertyChangeListener) {
184-
listeners.add(listener)
189+
synchronized(listenersLock) { listeners.add(listener) }
185190
}
186191

187192
/**
188193
* Removes a property change listener from this semaphore.
189194
*
195+
* **Thread Safety Note**: This method is synchronized to allow safe listener unregistration
196+
* from multiple threads, even though the simulation context itself is not thread-safe.
197+
*
190198
* @param listener The listener to remove
191199
*/
192200
fun removePropertyChangeListener(listener: ContextPropertyChangeListener) {
193-
listeners.remove(listener)
201+
synchronized(listenersLock) { listeners.remove(listener) }
194202
}
195203

196204
/**

‎src/main/kotlin/cz/vutbr/fit/interlockSim/objects/cells/DynamicRailSwitch.kt‎

Lines changed: 17 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -76,6 +76,7 @@ class DynamicRailSwitch(
7676
* Listeners for switch state changes (Kotlin-native, no java.beans dependency).
7777
*/
7878
private val listeners = mutableListOf<ContextPropertyChangeListener>()
79+
private val listenersLock = Any()
7980

8081
/**
8182
* Changes the switch configuration to the opposite position.
@@ -94,7 +95,8 @@ class DynamicRailSwitch(
9495
logger.info {
9596
"${jDisco.Process.time()} Switch ${staticRef.hashCode()} position change: $oldConf -> $conf"
9697
}
97-
listeners.toList().forEach { it.propertyChange(ContextChangeEvent("conf", oldConf, conf)) }
98+
val snapshot = synchronized(listenersLock) { listeners.toList() }
99+
snapshot.forEach { it.propertyChange(ContextChangeEvent("conf", oldConf, conf)) }
98100
}
99101

100102
override fun cancelPathSetup(
@@ -132,7 +134,8 @@ class DynamicRailSwitch(
132134
}
133135
conf = newConf
134136
if (oldConf != newConf) {
135-
listeners.toList().forEach { it.propertyChange(ContextChangeEvent("conf", oldConf, newConf)) }
137+
val snapshot = synchronized(listenersLock) { listeners.toList() }
138+
snapshot.forEach { it.propertyChange(ContextChangeEvent("conf", oldConf, newConf)) }
136139
}
137140
// Tier 1: Lock switch after configuration (Issue #291)
138141
lock()
@@ -179,7 +182,8 @@ class DynamicRailSwitch(
179182
logger.debug {
180183
"${jDisco.Process.time()} Switch ${staticRef.hashCode()} locked"
181184
}
182-
listeners.toList().forEach { it.propertyChange(ContextChangeEvent("locked", oldLocked, locked)) }
185+
val snapshot = synchronized(listenersLock) { listeners.toList() }
186+
snapshot.forEach { it.propertyChange(ContextChangeEvent("locked", oldLocked, locked)) }
183187
}
184188

185189
/**
@@ -196,7 +200,8 @@ class DynamicRailSwitch(
196200
logger.debug {
197201
"${jDisco.Process.time()} Switch ${staticRef.hashCode()} unlocked"
198202
}
199-
listeners.toList().forEach { it.propertyChange(ContextChangeEvent("locked", oldLocked, locked)) }
203+
val snapshot = synchronized(listenersLock) { listeners.toList() }
204+
snapshot.forEach { it.propertyChange(ContextChangeEvent("locked", oldLocked, locked)) }
200205
}
201206

202207
/**
@@ -223,19 +228,25 @@ class DynamicRailSwitch(
223228
/**
224229
* Registers a listener to be notified of switch state changes.
225230
*
231+
* **Thread Safety Note**: This method is synchronized to allow safe listener registration
232+
* from multiple threads, even though the simulation context itself is not thread-safe.
233+
*
226234
* @param listener the listener to add
227235
*/
228236
fun addPropertyChangeListener(listener: ContextPropertyChangeListener) {
229-
listeners.add(listener)
237+
synchronized(listenersLock) { listeners.add(listener) }
230238
}
231239

232240
/**
233241
* Unregisters a listener from receiving switch state change notifications.
234242
*
243+
* **Thread Safety Note**: This method is synchronized to allow safe listener unregistration
244+
* from multiple threads, even though the simulation context itself is not thread-safe.
245+
*
235246
* @param listener the listener to remove
236247
*/
237248
fun removePropertyChangeListener(listener: ContextPropertyChangeListener) {
238-
listeners.remove(listener)
249+
synchronized(listenersLock) { listeners.remove(listener) }
239250
}
240251

241252
/**

‎src/main/kotlin/cz/vutbr/fit/interlockSim/objects/tracks/DynamicTrack.kt‎

Lines changed: 22 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -42,6 +42,7 @@ class DynamicTrack(
4242
val staticRef: TrackFacility
4343
) {
4444
private val listeners = mutableListOf<ContextPropertyChangeListener>()
45+
private val listenersLock = Any()
4546

4647
// Static properties delegated from wrapped object
4748
val length: Double
@@ -118,9 +119,10 @@ class DynamicTrack(
118119
occupant = newOccupant
119120
reservedFrom = null
120121
// Fire property change events
121-
listeners.toList().forEach { it.propertyChange(ContextChangeEvent("state", oldState, state)) }
122-
listeners.toList().forEach { it.propertyChange(ContextChangeEvent("occupant", null, newOccupant)) }
123-
listeners.toList().forEach { it.propertyChange(ContextChangeEvent("reservedFrom", oldReservedFrom, null)) }
122+
val snapshot = synchronized(listenersLock) { listeners.toList() }
123+
snapshot.forEach { it.propertyChange(ContextChangeEvent("state", oldState, state)) }
124+
snapshot.forEach { it.propertyChange(ContextChangeEvent("occupant", null, newOccupant)) }
125+
snapshot.forEach { it.propertyChange(ContextChangeEvent("reservedFrom", oldReservedFrom, null)) }
124126
}
125127

126128
/**
@@ -144,8 +146,9 @@ class DynamicTrack(
144146
assertGoodStateChange(TrackFacility.State.OCCUPIED, TrackFacility.State.FREE)
145147
occupant = null
146148
// Fire property change events
147-
listeners.toList().forEach { it.propertyChange(ContextChangeEvent("state", oldState, state)) }
148-
listeners.toList().forEach { it.propertyChange(ContextChangeEvent("occupant", oldOccupant, null)) }
149+
val snapshot = synchronized(listenersLock) { listeners.toList() }
150+
snapshot.forEach { it.propertyChange(ContextChangeEvent("state", oldState, state)) }
151+
snapshot.forEach { it.propertyChange(ContextChangeEvent("occupant", oldOccupant, null)) }
149152
}
150153

151154
/**
@@ -184,8 +187,9 @@ class DynamicTrack(
184187
exceptionStateChange(TrackFacility.State.FREE, TrackFacility.State.RESERVED)
185188
reservedFrom = sep
186189
// Fire property change events
187-
listeners.toList().forEach { it.propertyChange(ContextChangeEvent("state", oldState, state)) }
188-
listeners.toList().forEach { it.propertyChange(ContextChangeEvent("reservedFrom", null, sep)) }
190+
val snapshot = synchronized(listenersLock) { listeners.toList() }
191+
snapshot.forEach { it.propertyChange(ContextChangeEvent("state", oldState, state)) }
192+
snapshot.forEach { it.propertyChange(ContextChangeEvent("reservedFrom", null, sep)) }
189193
}
190194

191195
/**
@@ -249,8 +253,9 @@ class DynamicTrack(
249253
}
250254
reservedFrom = null
251255
// Fire property change events
252-
listeners.toList().forEach { it.propertyChange(ContextChangeEvent("state", oldState, state)) }
253-
listeners.toList().forEach { it.propertyChange(ContextChangeEvent("reservedFrom", oldReservedFrom, null)) }
256+
val snapshot = synchronized(listenersLock) { listeners.toList() }
257+
snapshot.forEach { it.propertyChange(ContextChangeEvent("state", oldState, state)) }
258+
snapshot.forEach { it.propertyChange(ContextChangeEvent("reservedFrom", oldReservedFrom, null)) }
254259
}
255260

256261
// Private helper methods for state transitions
@@ -321,20 +326,26 @@ class DynamicTrack(
321326
* - "occupant" property: changes when train enters/leaves
322327
* - "reservedFrom" property: changes when path is set up/cancelled
323328
*
329+
* **Thread Safety Note**: This method is synchronized to allow safe listener registration
330+
* from multiple threads, even though the simulation context itself is not thread-safe.
331+
*
324332
* @param listener The listener to add
325333
* @see PropertyChangeSupport.addPropertyChangeListener
326334
*/
327335
fun addPropertyChangeListener(listener: ContextPropertyChangeListener) {
328-
listeners.add(listener)
336+
synchronized(listenersLock) { listeners.add(listener) }
329337
}
330338

331339
/**
332340
* Removes a property change listener from this track.
333341
*
342+
* **Thread Safety Note**: This method is synchronized to allow safe listener unregistration
343+
* from multiple threads, even though the simulation context itself is not thread-safe.
344+
*
334345
* @param listener The listener to remove
335346
*/
336347
fun removePropertyChangeListener(listener: ContextPropertyChangeListener) {
337-
listeners.remove(listener)
348+
synchronized(listenersLock) { listeners.remove(listener) }
338349
}
339350

340351
/**

‎src/test/kotlin/cz/vutbr/fit/interlockSim/objects/cells/DynamicRailSemaphoreTest.kt‎

Lines changed: 42 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -407,4 +407,46 @@ class DynamicRailSemaphoreTest {
407407
assertThat(dynamicSemaphore1.equals(staticSemaphore2)).isFalse()
408408
}
409409
}
410+
411+
@Nested
412+
@DisplayName("Listener thread-safety")
413+
inner class ListenerThreadSafety {
414+
@Test
415+
fun `listener added during event callback does not cause ConcurrentModificationException`() {
416+
// Regression test: listener mutation during event dispatch must not throw CME.
417+
val secondListenerCallCount = intArrayOf(0)
418+
val secondListener = ContextPropertyChangeListener { secondListenerCallCount[0]++ }
419+
420+
val firstListener = ContextPropertyChangeListener {
421+
dynamicSemaphore1.addPropertyChangeListener(secondListener)
422+
}
423+
dynamicSemaphore1.addPropertyChangeListener(firstListener)
424+
425+
// Should not throw ConcurrentModificationException (snapshot prevents it)
426+
dynamicSemaphore1.signal = Signal.FREE
427+
428+
// Fire again — second listener is now registered and should receive the event
429+
dynamicSemaphore1.signal = Signal.STOP
430+
assertThat(secondListenerCallCount[0]).isGreaterThan(0)
431+
}
432+
433+
@Test
434+
fun `listener removed during event callback does not cause ConcurrentModificationException`() {
435+
// Regression test: listener self-removal during dispatch must be safe (snapshot).
436+
var callCount = 0
437+
lateinit var selfRemovingListener: ContextPropertyChangeListener
438+
selfRemovingListener = ContextPropertyChangeListener {
439+
callCount++
440+
dynamicSemaphore1.removePropertyChangeListener(selfRemovingListener)
441+
}
442+
dynamicSemaphore1.addPropertyChangeListener(selfRemovingListener)
443+
444+
dynamicSemaphore1.signal = Signal.FREE
445+
val countAfterFirstFire = callCount
446+
447+
// Listener was removed; further fires must not call it again
448+
dynamicSemaphore1.signal = Signal.STOP
449+
assertThat(callCount).isEqualTo(countAfterFirstFire)
450+
}
451+
}
410452
}

‎src/test/kotlin/cz/vutbr/fit/interlockSim/objects/cells/DynamicRailSwitchTest.kt‎

Lines changed: 43 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -15,6 +15,7 @@ import assertk.assertions.*
1515
import cz.vutbr.fit.interlockSim.objects.cells.RailSwitch.Conf
1616
import cz.vutbr.fit.interlockSim.objects.cells.RailSwitch.Type
1717
import cz.vutbr.fit.interlockSim.objects.core.Cell
18+
import cz.vutbr.fit.interlockSim.objects.core.ContextPropertyChangeListener
1819
import cz.vutbr.fit.interlockSim.testutil.providers.SwitchActiveSegmentsProvider
1920
import org.junit.jupiter.api.BeforeEach
2021
import org.junit.jupiter.api.DisplayName
@@ -400,4 +401,46 @@ class DynamicRailSwitchTest {
400401
assertThat(dynamicSwitch1.equals(staticSwitch2)).isFalse()
401402
}
402403
}
404+
405+
@Nested
406+
@DisplayName("Listener thread-safety")
407+
inner class ListenerThreadSafety {
408+
@Test
409+
fun `listener added during event callback does not cause ConcurrentModificationException`() {
410+
// Regression test: listener mutation during event dispatch must not throw CME.
411+
val secondListenerCallCount = intArrayOf(0)
412+
val secondListener = ContextPropertyChangeListener { secondListenerCallCount[0]++ }
413+
414+
val firstListener = ContextPropertyChangeListener {
415+
dynamicSwitch1.addPropertyChangeListener(secondListener)
416+
}
417+
dynamicSwitch1.addPropertyChangeListener(firstListener)
418+
419+
// Should not throw ConcurrentModificationException (snapshot prevents it)
420+
dynamicSwitch1.changeConf()
421+
422+
// Fire again — second listener is now registered and should receive the event
423+
dynamicSwitch1.changeConf()
424+
assertThat(secondListenerCallCount[0]).isGreaterThan(0)
425+
}
426+
427+
@Test
428+
fun `listener removed during event callback does not cause ConcurrentModificationException`() {
429+
// Regression test: listener self-removal during dispatch must be safe (snapshot).
430+
var callCount = 0
431+
lateinit var selfRemovingListener: ContextPropertyChangeListener
432+
selfRemovingListener = ContextPropertyChangeListener {
433+
callCount++
434+
dynamicSwitch1.removePropertyChangeListener(selfRemovingListener)
435+
}
436+
dynamicSwitch1.addPropertyChangeListener(selfRemovingListener)
437+
438+
dynamicSwitch1.changeConf()
439+
val countAfterFirstFire = callCount
440+
441+
// Listener was removed; further fires must not call it again
442+
dynamicSwitch1.changeConf()
443+
assertThat(callCount).isEqualTo(countAfterFirstFire)
444+
}
445+
}
403446
}

‎src/test/kotlin/cz/vutbr/fit/interlockSim/objects/tracks/DynamicTrackTest.kt‎

Lines changed: 41 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -533,4 +533,45 @@ class DynamicTrackTest {
533533
// Then: event should have been fired
534534
assertThat(capturedEvents).isNotEmpty()
535535
}
536+
537+
@Test
538+
fun `listener added during event callback does not cause ConcurrentModificationException`() {
539+
// Regression test for thread-safety fix: listener mutation during event dispatch
540+
// must not throw ConcurrentModificationException (snapshot prevents this).
541+
val secondListenerCallCount = intArrayOf(0)
542+
val secondListener = ContextPropertyChangeListener { secondListenerCallCount[0]++ }
543+
544+
// Listener that adds a second listener during its callback
545+
val firstListener = ContextPropertyChangeListener {
546+
dynamicTrack1.addPropertyChangeListener(secondListener)
547+
}
548+
dynamicTrack1.addPropertyChangeListener(firstListener)
549+
550+
// When: state changes - should not throw ConcurrentModificationException
551+
dynamicTrack1.setUpPath(separator1)
552+
553+
// Then: second listener was registered; fire again to verify it receives events
554+
dynamicTrack1.cancelPathSetup(separator1)
555+
assertThat(secondListenerCallCount[0]).isGreaterThan(0)
556+
}
557+
558+
@Test
559+
fun `listener removed during event callback does not cause ConcurrentModificationException`() {
560+
// Regression test: listener self-removal during event dispatch must be safe (snapshot).
561+
var callCount = 0
562+
lateinit var selfRemovingListener: ContextPropertyChangeListener
563+
selfRemovingListener = ContextPropertyChangeListener {
564+
callCount++
565+
dynamicTrack1.removePropertyChangeListener(selfRemovingListener)
566+
}
567+
dynamicTrack1.addPropertyChangeListener(selfRemovingListener)
568+
569+
// Should not throw; listener removes itself on first event
570+
dynamicTrack1.setUpPath(separator1)
571+
val countAfterFirstFire = callCount
572+
573+
// Listener was removed — second fire should not increment count
574+
dynamicTrack1.cancelPathSetup(separator1)
575+
assertThat(callCount).isEqualTo(countAfterFirstFire)
576+
}
536577
}

0 commit comments

Comments
 (0)