Some fixes discovered on high usage of Miou.call and Miou.Queue #36
Conversation
| let is_signaled t = Atomic.get t = Signaled | ||
| let is_initial t = Atomic.get t = Initial |
There was a problem hiding this comment.
Note that == and = will do the exact same thing in this case.
There was a problem hiding this comment.
Yes, but I prefer to be conservative on this space. I mean, it's hard to remember all reasons that it's currently safe to user == instead of = 😕.
There was a problem hiding this comment.
Note that Atomic.compare_and_set uses physical equality. I would generally recommend thinking in terms of physical equality with atomics unless there is very specific reason to prefer other forms of equality.
There was a problem hiding this comment.
This is indeed the model I could have had in the first place, but this issue still makes me break out in a cold sweat over the use of ==. Once again, the point is to facilitate rereading without reconsidering why it's okay to use == 10 years after you've done it!
| let rec drop t = | ||
| let ((head, tail) as snapshot) = snapshot t in | ||
| if Atomic.compare_and_set t.head head tail | ||
| then snapshot else drop t |
There was a problem hiding this comment.
This has a space leak. The problem is that, unless the queue is already empty, the node pointed to by tail still has a value and after a successful compare_and_set that value will still be pointed to by the queue.
There was a problem hiding this comment.
Actually, that's the bug I encountered on another project. I'm not sure how to implement a atomic drop unfortunately. I decided to switch to a mutex + a simple queue.
| then snapshot else drop t | ||
|
|
||
| let drop ~f t = iter ~f (drop t) | ||
| let iter ~f t = iter ~f (snapshot t) |
There was a problem hiding this comment.
Note that iter is not concurrency safe. If one thread calls iter ~f q and another thread performs dequeue q, then the ~f function may be called with Obj.magic ():
Line 63 in 9d18413
There was a problem hiding this comment.
More precisely, this is what I encountered on my project. I suspected that this version will help me but it's not - and it's why I decided to switch to a mutex + a simple queue. If you have an idea to have an atomic iter, it will be superb but my brain is actually focus and limited on some others stuffs.
CHANGES: - Set the internal pipe used to interrupt a domain to a non-blocking mode and remove the usage of an atomic which protect how we fill the pipe (@haesbaert, @dinosaure, robur-coop/miou#28) - Expose option to reuse addr/port when we `Miou_unix.bind_and_listen` (@ada2k, @dinosaure, robur-coop/miou#27) - Protect an illegal access to the orphan from a possibly parallel task which does not own the orphan value (@poytypic, @dinosaure, robur-coop/miou#31, robur-coop/miou#32) - Be able to pin a specific domain when we want to launch a parallel task (@dinosaure, robur-coop/miou#34) - Expose the `Miou.Backoff` module which can be useful for users (@dinosaure, robur-coop/miou#35) - Fix or improve (from the maintainance point-of-view) the `Miou.Queue` module and some internal parts of Miou about the usage of atomics (@dinosaure, @polytypic, robur-coop/miou#36, robur-coop/miou#33) - Prefer to require a `finaliser` function for the `events` value and actually close the internal `Unix.pipe` used to interrupt domain than to use `Gc.finaliser` and possibly leak file-descriptors (spotted by @hannesm, @dinosaure, robur-coop/miou#37)
CHANGES: - Set the internal pipe used to interrupt a domain to a non-blocking mode and remove the usage of an atomic which protect how we fill the pipe (@haesbaert, @dinosaure, robur-coop/miou#28) - Expose option to reuse addr/port when we `Miou_unix.bind_and_listen` (@ada2k, @dinosaure, robur-coop/miou#27) - Protect an illegal access to the orphan from a possibly parallel task which does not own the orphan value (@poytypic, @dinosaure, robur-coop/miou#31, robur-coop/miou#32) - Be able to pin a specific domain when we want to launch a parallel task (@dinosaure, robur-coop/miou#34) - Expose the `Miou.Backoff` module which can be useful for users (@dinosaure, robur-coop/miou#35) - Fix or improve (from the maintainance point-of-view) the `Miou.Queue` module and some internal parts of Miou about the usage of atomics (@dinosaure, @polytypic, robur-coop/miou#36, robur-coop/miou#33) - Prefer to require a `finaliser` function for the `events` value and actually close the internal `Unix.pipe` used to interrupt domain than to use `Gc.finaliser` and possibly leak file-descriptors (spotted by @hannesm, @dinosaure, robur-coop/miou#37)
No description provided.