Skip to content

Commit 74599ce

Browse files
CopilotbedaHovorka
andauthored
Document Context as not thread-safe by design (#75)
Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: bedaHovorka <5263405+bedaHovorka@users.noreply.github.com>
1 parent 1e246bf commit 74599ce

5 files changed

Lines changed: 146 additions & 25 deletions

File tree

src/main/kotlin/cz/vutbr/fit/interlockSim/context/Context.kt

Lines changed: 31 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -20,6 +20,37 @@ import java.beans.PropertyChangeListener
2020
* Interface to shared functions of inner data model, which is allowed allways
2121
*
2222
* Point is used as Pair of integers
23+
*
24+
* ## Thread Safety
25+
*
26+
* **This interface and its implementations are NOT thread-safe.**
27+
*
28+
* Context instances must not be accessed from multiple threads concurrently.
29+
* All operations on the railway network grid, graph structure, and property
30+
* change notifications assume single-threaded access.
31+
*
32+
* If concurrent access is required, external synchronization must be used to
33+
* ensure thread safety.
34+
*
35+
* ### Rationale
36+
*
37+
* Railway interlocking simulations are inherently sequential by design:
38+
* - The jDisco discrete event simulation framework operates in a single thread
39+
* - Physical railway operations follow sequential causality (trains cannot
40+
* simultaneously occupy the same track)
41+
* - The simulation model enforces discrete event ordering
42+
*
43+
* Thread-safety mechanisms would introduce unnecessary complexity and performance
44+
* overhead without providing practical benefits for the intended use cases.
45+
*
46+
* ### Usage Guidelines
47+
*
48+
* - **DO NOT** access Context from multiple threads
49+
* - **DO NOT** share Context instances across thread boundaries
50+
* - **DO** use external synchronization if multi-threaded access is unavoidable
51+
* - **DO** keep all Context operations within the simulation thread
52+
*
53+
* @see javax.annotation.concurrent.NotThreadSafe
2354
*/
2455
interface Context {
2556
/**

src/main/kotlin/cz/vutbr/fit/interlockSim/context/DefaultContext.kt

Lines changed: 35 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -54,6 +54,41 @@ import java.util.TreeMap
5454

5555
/**
5656
* implementation of {@link EditingContext} and {@link SimulationContext}
57+
*
58+
* ## Thread Safety
59+
*
60+
* **This class is NOT thread-safe.**
61+
*
62+
* DefaultContext maintains mutable state including:
63+
* - Railway network grid (DefaultRailWayNetGrid)
64+
* - Graph structure (ExtendedUnorientedGraph)
65+
* - Property change listeners (PropertyChangeSupport)
66+
* - Track block mappings and simulation state
67+
*
68+
* These data structures are not synchronized and will experience race conditions
69+
* if accessed concurrently from multiple threads. Observed issues include:
70+
* - NullPointerException in grid access
71+
* - ArrayIndexOutOfBoundsException in cell operations
72+
* - ConcurrentModificationException in listener notifications
73+
*
74+
* ### Design Decision
75+
*
76+
* Thread safety is intentionally NOT implemented because:
77+
* 1. Railway simulations are inherently sequential (discrete event model)
78+
* 2. The jDisco framework operates in a single thread
79+
* 3. No current use cases require concurrent access
80+
* 4. Thread-safety would add complexity and performance overhead
81+
*
82+
* ### Usage
83+
*
84+
* - Access DefaultContext only from the simulation/editing thread
85+
* - Do not share instances across thread boundaries
86+
* - Use external synchronization if multi-threaded access is unavoidable
87+
*
88+
* @see Context
89+
* @see EditingContext
90+
* @see SimulationContext
91+
* @see javax.annotation.concurrent.NotThreadSafe
5792
*/
5893
abstract class DefaultContext :
5994
EditingContext,

src/main/kotlin/cz/vutbr/fit/interlockSim/context/EditingContext.kt

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -16,6 +16,16 @@ import cz.vutbr.fit.interlockSim.util.Point
1616
/**
1717
* Interface to shared functions of inner data model, which is allowed by editing
1818
*
19+
* ## Thread Safety
20+
*
21+
* **This interface is NOT thread-safe.** See [Context] for detailed thread safety
22+
* documentation and usage guidelines.
23+
*
24+
* All editing operations (putCell, removeCell, moveCell, joinCells, removeLine)
25+
* must be performed from a single thread.
26+
*
27+
* @see Context
28+
* @see javax.annotation.concurrent.NotThreadSafe
1929
*/
2030
interface EditingContext : Context {
2131
/**

src/main/kotlin/cz/vutbr/fit/interlockSim/context/SimulationContext.kt

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -25,6 +25,17 @@ import java.util.EnumSet
2525
/**
2626
* Interface to shared functions of inner data model, which is allowed by simulation
2727
*
28+
* ## Thread Safety
29+
*
30+
* **This interface is NOT thread-safe.** See [Context] for detailed thread safety
31+
* documentation and usage guidelines.
32+
*
33+
* All simulation operations must be performed within the jDisco simulation thread.
34+
* The jDisco discrete event simulation framework is single-threaded by design,
35+
* ensuring sequential execution of all simulation events.
36+
*
37+
* @see Context
38+
* @see javax.annotation.concurrent.NotThreadSafe
2839
*/
2940
interface SimulationContext : Context {
3041
/**

src/test/kotlin/cz/vutbr/fit/interlockSim/RaceConditionTest.kt

Lines changed: 59 additions & 25 deletions
Original file line numberDiff line numberDiff line change
@@ -24,6 +24,7 @@ import cz.vutbr.fit.interlockSim.testutil.withMessage
2424
import cz.vutbr.fit.interlockSim.testutil.KoinTestBase
2525
import cz.vutbr.fit.interlockSim.testutil.buildLinearTrack
2626
import cz.vutbr.fit.interlockSim.testutil.buildMinimal
27+
import org.junit.jupiter.api.Disabled
2728
import org.junit.jupiter.api.DisplayName
2829
import org.junit.jupiter.api.Nested
2930
import org.junit.jupiter.api.Tag
@@ -38,37 +39,37 @@ import java.util.concurrent.atomic.AtomicReference
3839
/**
3940
* Comprehensive race condition tests for concurrent access scenarios.
4041
*
41-
* Tests the thread safety of core railway interlocking operations under
42-
* concurrent access patterns. These advanced tests validate that public APIs
43-
* properly handle simultaneous operations from multiple threads.
42+
* **IMPORTANT: Tests in this class are DISABLED by design.**
4443
*
45-
* Key scenarios tested:
46-
* 1. Concurrent path reservation attempts
47-
* 2. Simultaneous switch state changes
48-
* 3. Concurrent track occupancy updates
49-
* 4. Thread safety of Context modifications
44+
* These tests demonstrate race conditions that occur when Context classes are
45+
* accessed concurrently. However, Context is intentionally NOT thread-safe:
5046
*
51-
* Test organization:
52-
* - Concurrent path operations (reservation, setup, cancellation)
53-
* - Switch state modifications under concurrent access
54-
* - Track occupancy and entry/leave operations
55-
* - Context grid modifications during concurrent access
47+
* ## Design Decision (Option 1)
5648
*
57-
* Safety Properties Validated:
58-
* - SI-4: Path reservation atomicity (all-or-nothing)
59-
* - SI-5: Switch locking during path use (no state corruption)
60-
* - SI-1: No collision detection (track occupancy integrity)
61-
* - Data consistency under concurrent access
49+
* Context and its implementations (DefaultContext, EditingContext, SimulationContext)
50+
* are documented as NOT thread-safe. Multi-threaded access is not supported.
6251
*
63-
* Thread Safety Strategy:
64-
* - Use CountDownLatch for precise synchronization
65-
* - Use AtomicInteger/AtomicBoolean for thread-safe counters
66-
* - Use CopyOnWriteArrayList for thread-safe exception collection
67-
* - Avoid timing-dependent assertions (use latches instead)
68-
* - Conservative API testing (test only public methods)
52+
* ### Rationale:
53+
* 1. Railway simulations are inherently sequential (discrete event model)
54+
* 2. The jDisco framework is single-threaded by design
55+
* 3. No current use cases require concurrent Context access
56+
* 4. Thread-safety would add unnecessary complexity and performance overhead
57+
* 5. Aligns with KISS principle
6958
*
70-
* Coverage Target: ~50-75 instructions
59+
* ### Race Conditions Documented:
60+
* - NullPointerException in grid access during concurrent modifications
61+
* - ArrayIndexOutOfBoundsException in cell operations
62+
* - ConcurrentModificationException in listener notifications
7163
*
64+
* These tests remain in the codebase to:
65+
* 1. Document known race conditions if thread-safety is attempted in the future
66+
* 2. Provide evidence for the design decision to not implement thread-safety
67+
* 3. Serve as integration tests if the design decision is revisited
68+
*
69+
* **Usage:** Do not enable these tests unless implementing thread-safety (Option 2).
70+
*
71+
* @see cz.vutbr.fit.interlockSim.context.Context
72+
* @see cz.vutbr.fit.interlockSim.context.DefaultContext
7273
* @since 2026-01 (Phase 6 advanced testing)
7374
* @tag integration-test
7475
*/
@@ -632,8 +633,19 @@ class RaceConditionTest : KoinTestBase() {
632633
inner class ConcurrentContextModifications {
633634
/**
634635
* Test concurrent cell placement and retrieval from the context grid.
636+
*
637+
* **DISABLED: Context is not thread-safe by design.**
638+
*
639+
* This test demonstrates ArrayIndexOutOfBoundsException that occurs when
640+
* multiple threads concurrently access the Context grid. This is expected
641+
* behavior as Context is intentionally not thread-safe.
642+
*
643+
* See Context documentation for rationale and usage guidelines.
644+
*
645+
* @see <a href="https://github.com/bedaHovorka/interlockSim/issues/28">Issue #28</a>
635646
*/
636647
@Test
648+
@Disabled("Context is not thread-safe by design (Issue #28). Do not use from multiple threads.")
637649
@DisplayName("Concurrent cell operations on context grid are thread-safe")
638650
fun concurrentCellOperations_contextGrid_threadSafe() {
639651
// Arrange
@@ -692,8 +704,19 @@ class RaceConditionTest : KoinTestBase() {
692704

693705
/**
694706
* Test concurrent context modifications maintain grid consistency.
707+
*
708+
* **DISABLED: Context is not thread-safe by design.**
709+
*
710+
* This test demonstrates NullPointerException that occurs when multiple
711+
* threads concurrently modify the Context grid. This is expected behavior
712+
* as Context is intentionally not thread-safe.
713+
*
714+
* See Context documentation for rationale and usage guidelines.
715+
*
716+
* @see <a href="https://github.com/bedaHovorka/interlockSim/issues/28">Issue #28</a>
695717
*/
696718
@Test
719+
@Disabled("Context is not thread-safe by design (Issue #28). Do not use from multiple threads.")
697720
@DisplayName("Concurrent context modifications maintain grid consistency")
698721
fun concurrentContextMod_maintainsGridConsistency() {
699722
// Arrange
@@ -950,8 +973,19 @@ class RaceConditionTest : KoinTestBase() {
950973
/**
951974
* Test that multiple listener registrations and removals work correctly
952975
* under concurrent grid modifications.
976+
*
977+
* **DISABLED: Context is not thread-safe by design.**
978+
*
979+
* This test demonstrates ConcurrentModificationException that occurs when
980+
* listener notifications happen concurrently with grid modifications. This
981+
* is expected behavior as Context is intentionally not thread-safe.
982+
*
983+
* See Context documentation for rationale and usage guidelines.
984+
*
985+
* @see <a href="https://github.com/bedaHovorka/interlockSim/issues/28">Issue #28</a>
953986
*/
954987
@Test
988+
@Disabled("Context is not thread-safe by design (Issue #28). Do not use from multiple threads.")
955989
@DisplayName("Listeners and grid modifications work correctly concurrently")
956990
fun listenersAndGridMods_workConcurrently() {
957991
// Arrange

0 commit comments

Comments
 (0)