Skip to content

timesync: fix _blocking data race, reject sync/async handler switch at runtime - #453

Open
snps-fricke wants to merge 5 commits into
mainfrom
dev/step_handler_registration_race
Open

snps-fricke wants to merge 5 commits into
mainfrom
dev/step_handler_registration_race

Conversation

@snps-fricke

@snps-fricke snps-fricke commented Oct 28, 2026 •

Copy link
Copy Markdown
Collaborator

Description

Follow-up to #352 with two independent changes:

  1. Protect TimeConfiguration::_blocking with the mutex.
    Re-registering the async step handler from a user thread while the step is still open (the supported way to change the step size) races with ExecuteSimStep(), which reads the flag after the handler returned.
    See 367811e for a reproducer. It passes in a normal build and is reported by TSan without this fix.

  2. Reject switching between SetSimulationStepHandler and SetSimulationStepHandlerAsync once the participant is Running (SilKit::StateError).
    Switching before Running and re-registering a handler of the same kind at any time (e.g. to change the step size) is still allowed.

    Why: the mutex from 1. does not make the switch safe, a single step still reads the flag several times (IsBlocking(), IsSimStepSync()) and can see different values.

    • Most significant: a sync handler can run while the previous async step is still open. After the async handler returned and before CompleteSimulationStep(), a switch to sync makes the next NextSimTask take AdvanceTimeSimStepSync(), which, unlike AdvanceTimeSimStepAsync(), does not check _isExecutingSimStep.
      See temporary commit a908a6a for reproducers.
    • Otherwise non-functional issues: wrong step metrics in ExecuteSimStep(), stale EvaluateAggregationInfo() results.

    Rejecting this technically breaks compatibility, though I think it is unlikely that users switch between sync/async handlers at runtime. However, since re-registration is the advertised API for changing the step size, this became somewhat more prominent.

I think 1. is straightforward, 2. may need discussion. An alternative to rejecting may be to guard AdvanceTimeSimStepSync() with _isExecutingSimStep and fix metrics/aggregation separately.

Developer checklist (address before review)

  • Changelog.md updated
  • Prepared update for depending repositories
  • Documentation updated (public API changes only)
  • API docstrings updated (public API changes only)
  • Rebase → commit history clean
  • Squash and merge → proper PR title

@snps-fricke snps-fricke self-assigned this Oct 28, 2026
@snps-fricke snps-fricke added the status: needs discussion Proposal for internal changes. We need to agree on the approach before a detailed review. label Oct 28, 2026
Signed-off-by: Maximilian Fricke <maximilian.fricke@synopsys.com>
Signed-off-by: Maximilian Fricke <maximilian.fricke@synopsys.com>
Signed-off-by: Maximilian Fricke <maximilian.fricke@synopsys.com>
Signed-off-by: Maximilian Fricke <maximilian.fricke@synopsys.com>
Signed-off-by: Maximilian Fricke <maximilian.fricke@synopsys.com>
@snps-fricke
snps-fricke force-pushed the dev/step_handler_registration_race branch from a243258 to 905a8d6 Compare October 28, 2026 14:24

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

status: needs discussion Proposal for internal changes. We need to agree on the approach before a detailed review.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant