Skip to content

AsyncLib: Port data race fix from GDK w/ AsyncBlock written after AsyncGetStatus. - #1008

Open
ernadeau wants to merge 1 commit into
mainfrom
user/ernadeau/xasyncfix
Open

AsyncLib: Port data race fix from GDK w/ AsyncBlock written after AsyncGetStatus.#1008
ernadeau wants to merge 1 commit into
mainfrom
user/ernadeau/xasyncfix

Conversation

@ernadeau

Copy link
Copy Markdown

[PORT OF A FIX MADE TO 2604 GDK]

Why is this change being made?
XAsyncComplete published the block's "dead" signature (0 / ASYNC_BLOCK_RESULT_SIG ) from ExtractState while still holding the spin-lock and with pending writes. DoLock 's unlocked fast path skips locking when signature != ASYNC_BLOCK_SIG , so a concurrent XAsyncGetStatus(wait=true) could see terminal status + null state, return, and let the app free the block before the completer's final lock.clear() landed - a use-after-free confirmed by a live repro (write to the freed user block, asynclib lock.clear() at completion).

What changed?
Made AsyncBlockInternal::signature a std::atomic<uint32_t> and deferred the dead-signature publish: the guard destructor now stores it (release) as its final write, after both lock.clear() s, and readers acquire-load it. DoLock gained a retry loop that, on a locked block whose state is already extracted but whose dead signature isn't yet published, waits for that store before returning null - closing the race. Added regression test VerifyNoWriteAfterGetStatusRace (20k iterations, byte-identical block); full suite 90/90.

How was the change tested?

  • The original bug reported by a developer was reproduced with a synthetic GDK test. Validated that the changes fix the bug on PC with the test.
  • Compiles clean: 0 warnings, 0 errors;  AsyncLib.cpp  +  AsyncBlockTests.cpp  built and linked.
  • New regression test passes consistently (20k iterations, ~0.9s).
  • Full AsyncBlockTests suite: 24/24 pass. One run showed a transient  VerifyGetAsyncStatusNoDeadlock  failure - in that test's own destructor global-state-counter check (a background completion still draining), not in test logic or our code. It passed 5/5 in isolation and 24/24 on both subsequent full-batch runs, confirming it's pre-existing teardown flakiness, not a regression from the port.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant