Skip to content

fix: avoid possible data leak while rendering#41

Closed
alekseysotnikov wants to merge 1 commit into
dynamic-alpha:mainfrom
alekseysotnikov:fix/avoid-possible-data-leak-while-rendering
Closed

fix: avoid possible data leak while rendering#41
alekseysotnikov wants to merge 1 commit into
dynamic-alpha:mainfrom
alekseysotnikov:fix/avoid-possible-data-leak-while-rendering

Conversation

@alekseysotnikov
Copy link
Copy Markdown
Contributor

Between separate read and the reset another thread can add scripts/dirty-ids/full-render flags that get silently overwritten ("leaked").

swap-vals! atomically reads the old value and sets the new, closing the race window.

Replace non-atomic deref + reset! with swap-vals! for the render
loop's full-render-needed*, pending-partials*, and
drain-pending-scripts!'s pending-scripts* atoms.
Between @deref and reset!, another thread could append to the atom
— those entries would be silently overwritten by reset! and lost.
swap-vals! atomically reads the old value and sets the new, closing
the race window.
@rschmukler
Copy link
Copy Markdown
Contributor

@alekseysotnikov thanks for catching this - it was a real bug.

I ended up going a step further and replacing the atoms + semaphore with a single LinkedBlockingQueue (hyper.render.queue). The swap-vals! fix is correct per-atom, but with three separate atoms there's still a cross-atom inconsistency window (e.g. reading full-render-needed* as false, then another thread sets it to true before we read pending-partials* - we'd do a partial when a full was requested). It's self-healing since the semaphore wakes the loop again, but the queue approach just makes the whole class of problem go away. This refactor makes it so we effectively don't have any shared mutable state between threads.

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.

2 participants