Skip to content

Commit 8e1f6e5

Browse files
committed
fix global state pollution
1 parent 790531c commit 8e1f6e5

3 files changed

Lines changed: 67 additions & 16 deletions

File tree

lib/actor_simulation.ex

Lines changed: 4 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -167,15 +167,13 @@ defmodule ActorSimulation do
167167
args = Keyword.get(opts, :args, nil)
168168
targets = Keyword.get(opts, :targets, [])
169169

170-
# Enable stats tracking before starting the process
171-
VirtualTimeGenServer.enable_stats_tracking()
172-
173-
# Start the real GenServer with the simulation's local virtual clock
174-
# This avoids setting the global clock and affecting other tests
170+
# Start the real GenServer with the simulation's local virtual clock and stats injection
171+
# This avoids setting global state and affecting other tests
175172
{:ok, pid} =
176173
VirtualTimeGenServer.start_link(module, args,
177174
name: name,
178-
virtual_clock: simulation.clock
175+
virtual_clock: simulation.clock,
176+
stats_enabled: true
179177
)
180178

181179
# Set up actor name context for trace generation

lib/virtual_time_gen_server.ex

Lines changed: 61 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -484,13 +484,14 @@ defmodule VirtualTimeGenServer do
484484
# Extract time-related options from opts
485485
{virtual_clock, opts} = Keyword.pop(opts, :virtual_clock)
486486
{real_time, opts} = Keyword.pop(opts, :real_time, false)
487+
{stats_enabled, opts} = Keyword.pop(opts, :stats_enabled, false)
487488

488489
# Determine which clock and backend to use
489490
# Priority: local options > global Process dictionary
490491
{final_clock, final_backend} = determine_time_config(virtual_clock, real_time)
491492

492-
# Get stats tracking flag from parent process
493-
stats_enabled = Process.get(:__vtgs_stats_enabled__, false)
493+
# Use injected stats_enabled option, fallback to parent process if not provided
494+
final_stats_enabled = stats_enabled || Process.get(:__vtgs_stats_enabled__, false)
494495

495496
# Use a wrapper to inject virtual clock into spawned process
496497
init_fun = fn ->
@@ -501,7 +502,7 @@ defmodule VirtualTimeGenServer do
501502
Process.put(:time_backend, final_backend)
502503

503504
# Propagate stats tracking to child process
504-
if stats_enabled do
505+
if final_stats_enabled do
505506
Process.put(:__vtgs_stats_enabled__, true)
506507
Process.put(:__vtgs_stats__, %{sent_count: 0, received_count: 0})
507508
end
@@ -526,12 +527,13 @@ defmodule VirtualTimeGenServer do
526527
# Extract time-related options from opts
527528
{virtual_clock, opts} = Keyword.pop(opts, :virtual_clock)
528529
{real_time, opts} = Keyword.pop(opts, :real_time, false)
530+
{stats_enabled, opts} = Keyword.pop(opts, :stats_enabled, false)
529531

530532
# Determine which clock and backend to use
531533
{final_clock, final_backend} = determine_time_config(virtual_clock, real_time)
532534

533-
# Get stats tracking flag from parent process
534-
stats_enabled = Process.get(:__vtgs_stats_enabled__, false)
535+
# Use injected stats_enabled option, fallback to parent process if not provided
536+
final_stats_enabled = stats_enabled || Process.get(:__vtgs_stats_enabled__, false)
535537

536538
init_fun = fn ->
537539
if final_clock do
@@ -541,7 +543,7 @@ defmodule VirtualTimeGenServer do
541543
Process.put(:time_backend, final_backend)
542544

543545
# Propagate stats tracking to child process
544-
if stats_enabled do
546+
if final_stats_enabled do
545547
Process.put(:__vtgs_stats_enabled__, true)
546548
Process.put(:__vtgs_stats__, %{sent_count: 0, received_count: 0})
547549
end
@@ -629,9 +631,60 @@ defmodule VirtualTimeGenServer do
629631
end
630632
end
631633

632-
@doc false
633-
# Internal API for ActorSimulation to enable stats tracking
634+
@doc """
635+
Sets GLOBAL stats tracking for all child processes.
636+
637+
⚠️ WARNING: This can cause race conditions in tests and production!
638+
639+
Consider using test-local stats injection instead:
640+
641+
```elixir
642+
# ❌ Global (can cause race conditions)
643+
VirtualTimeGenServer.enable_stats_tracking()
644+
{:ok, server} = MyServer.start_link([])
645+
646+
# ✅ Test-local (isolated, safe)
647+
{:ok, server} = MyServer.start_link([], stats_enabled: true)
648+
```
649+
650+
For coordinated stats tracking, use global tracking intentionally.
651+
For isolated testing, use test-local stats injection.
652+
"""
634653
def enable_stats_tracking do
654+
# Get caller information from stacktrace
655+
caller_info = get_caller_info()
656+
657+
# Emit a compilation warning to alert users about potential race conditions
658+
IO.warn(
659+
"""
660+
⚠️ GLOBAL STATS TRACKING DETECTED ⚠️
661+
662+
VirtualTimeGenServer.enable_stats_tracking/0 sets GLOBAL stats tracking that affects
663+
ALL child processes. This can cause race conditions in tests and production!
664+
665+
Consider using test-local stats injection instead:
666+
667+
# ❌ Global (can cause race conditions)
668+
VirtualTimeGenServer.enable_stats_tracking()
669+
{:ok, server} = MyServer.start_link([])
670+
671+
# ✅ Test-local (isolated, safe)
672+
{:ok, server} = MyServer.start_link([], stats_enabled: true)
673+
674+
For coordinated stats tracking, use global tracking intentionally.
675+
For isolated testing, use test-local stats injection.
676+
""",
677+
caller_info
678+
)
679+
680+
Process.put(:__vtgs_stats_enabled__, true)
681+
Process.put(:__vtgs_stats__, %{sent_count: 0, received_count: 0})
682+
end
683+
684+
@doc """
685+
Warning-free version of enable_stats_tracking/0 for intentional global usage.
686+
"""
687+
def enable_stats_tracking(:i_know_what_i_am_doing, explanation) when is_binary(explanation) do
635688
Process.put(:__vtgs_stats_enabled__, true)
636689
Process.put(:__vtgs_stats__, %{sent_count: 0, received_count: 0})
637690
end

test/actor_simulation_test.exs

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -107,12 +107,12 @@ defmodule ActorSimulationTest do
107107
targets: [:consumer]
108108
)
109109
|> ActorSimulation.add_actor(:consumer)
110-
|> ActorSimulation.run(duration: 1100)
110+
|> ActorSimulation.run(duration: 1000)
111111

112112
stats = ActorSimulation.get_stats(simulation)
113113

114114
# 2 bursts (at 0ms and 500ms), each with 5 messages = 10 total
115-
# Extended duration to 1100ms to ensure second burst at 500ms is fully processed
115+
# Quiescence should ensure all scheduled events are processed
116116
assert stats.actors[:producer].sent_count == 10
117117
assert stats.actors[:consumer].received_count == 10
118118

0 commit comments

Comments
 (0)