Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Possibly wrong usage of atomic-box-set! #48

Closed
cireu opened this issue Jun 29, 2021 · 1 comment
Closed

Possibly wrong usage of atomic-box-set! #48

cireu opened this issue Jun 29, 2021 · 1 comment

Comments

@cireu
Copy link

cireu commented Jun 29, 2021

https://github.com/wingo/fibers/blob/master/fibers/scheduler.scm#L272

I see (atomic-box-set! box (+ (atomic-box-ref ...) ...)) pattern here, but IIUC this will cause race condition in multithreading context, the box maybe update after atomic-box-ref and before atomic-box-set! by other threads.

I think this implementation is correct

(define update-count-box (box)
  (let retry ((old-box-val (atomic-box-ref box)))
    (let ((new-val (logand (1+ old-box-val) #xffffFFFF))
          (cur-box-val (atomic-box-compare-and-swap! box old-box-val new-val)))
      (if (eq? cur-box-val new-val)
          new-val
          (retry cur-box-val)))))
@emixa-d
Copy link
Collaborator

emixa-d commented Sep 4, 2023

That's incorrect, you are using Lisp's defun instead of Scheme's define.

I think the old implementation is correct, because there is no multithreading here -- IIUC, the only thread modifying the scheduler's runcount box is the scheduler thread.

I'll need to look more carefully to verify this, but I think the bug here is a missing comment explaining this.

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

No branches or pull requests

2 participants