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

Add add_nonblocking, capacity and is_full for streams #790

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

xavierm02
Copy link

Having a non-blocking add is useful when adding from an asynchronous callback from C code (e.g. an event handler or timeout in Lablgtk) because having the fiber yield would result in an Effect.Unhandled exception.

The simplest solution is to make the stream unbounded by creating it with the max int as capacity, but that has the disadvantage of letting the stream take up a potentially unbounded amount of memory.

Another solution, when using a single domain, is to use the is_full (or capacity) added by this commit, as one can do e.g. (if Stream.is_full stream then ignore (Stream.take stream)); Stream.add stream item.

In a context with multiple domain, this may not work as the lock is released between the take and the add, and another domain might add an element between these two operations. This is why add_nonblocking was also added: defining it from within the module allows to do both operations without releasing the lock in between.

I'm not too sure about Sync.put_nonblocking, and it might be reasonable to just raise an Invalid_argument exception when add_nonblocking is called on a stream of capacity 0.

@xavierm02 xavierm02 marked this pull request as draft December 13, 2024 16:04
@xavierm02
Copy link
Author

Hm. The Sync.put_nonblocking is probably wrong since it sets the cell value to Slot while it's a producer so it should set it to Item...

@xavierm02 xavierm02 force-pushed the stream-add-nonblocking branch from db9eade to e359a09 Compare December 13, 2024 17:04
@xavierm02
Copy link
Author

xavierm02 commented Dec 13, 2024

I removed Sync.put_nonblocking. I'm not sure how to handle the In_transition case.
(take_nonblocking sets the cell's value to a Slot that rejects any value to force the producer to retry sending its value in another cell, but in put_nonblocking, we're supposed to set it to an Item so we can't do that)

@xavierm02 xavierm02 marked this pull request as ready for review December 13, 2024 17:15
@talex5
Copy link
Collaborator

talex5 commented Dec 14, 2024

capacity and is_full look useful to me.

add_nonblocking seems a bit complicated, and I'm not sure it's that useful:

  • With lablgtk the consumer will typically be in the same domain as the callback, so the race there isn't a problem.
  • Even with multiple domains, if length < 100 then add t x is good enough to prevent it growing without bound. In fact, since lablgtk is the only producer it will always work perfectly. If there were multiple producers, it could go slightly over, but not by more than the number of producer domains.
  • I'm also not sure what kind of events could be dropped. For e.g. keypresses, I think you'd want to keep everything the user enters. For events where you just need to know you need to update (e.g. pointer motion events), you can use Eio.Condition instead.

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