Skip to content

Add more locks in update_status#132

Open
giordano wants to merge 5 commits intomainfrom
mg/locks
Open

Add more locks in update_status#132
giordano wants to merge 5 commits intomainfrom
mg/locks

Conversation

@giordano
Copy link
Copy Markdown
Collaborator

@giordano giordano commented May 1, 2026

Fix #131.

@AntonOresten
Copy link
Copy Markdown

Thanks!

@giordano
Copy link
Copy Markdown
Collaborator Author

giordano commented May 1, 2026

I wish we could support Julia v1.11+, then we'd be able to use Lockable which makes using locks a lot simpler.

Add a simple compatibility shim for Julia v1.10 which doesn't have
`Base.Lockable`.
@giordano giordano requested review from maleadt and vchuravy May 1, 2026 16:18
@giordano
Copy link
Copy Markdown
Collaborator Author

giordano commented May 1, 2026

Ok, it turns out defining Lockable for Julia v1.10 wasn't much complicated, so I did that and we can use Lockable so we don't forget to acquire the lock every time it's necessary.

giordano added 2 commits May 1, 2026 17:33
We don't need to refer to the lock specifically, we can simplify refer to the
lockable object.  This also means we don't need to keep the lock objects around.
Copy link
Copy Markdown
Collaborator

@maleadt maleadt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure this actually fixes the issue. For update_status, what about taking a snapshot of the relevant dicts at the start of the function?

  function update_status()
      running_snapshot = Base.@lock test_lock copy(running_tests)
      isempty(running_snapshot) && return

      results_snapshot = Base.@lock results_lock copy(results)
      completed = length(results_snapshot)
      total = length(tests)

      # continue using snapshots
  end

Lockable is still useful to protect wrkr.io.

See #133

Comment thread src/ParallelTestRunner.jl Outdated
* Fix update_status race by snapshotting under the lock instead.

* Drop unnecessary Lockable wrapping of `tests`

It's read-only anyway.
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.

UndefRefError: update_status races on running_tests

3 participants