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

Allow to configure the max size for unbounded queue #2093

Closed
wants to merge 7 commits into from

Conversation

gnunicorn
Copy link

We are using the unbounded queue for its nice non-blocking behavior, but it being a potentially-endless sink for data and thus memory leaks, if the other end isn't polled properly makes it harder to reason about the code.

This adds a buffer parameter analogous as to the bounded channel to allow the user of the API to ensure the queue is never overflowing more than a predefined number of items.

Right now this just errors with Full if an attempt to send is made but the max number is reached. I am thinking about allowing to define other strategies, like dropping that new entry or dropping the first entry to create ring-queues – depending on the usage scenarios, these might be good coping strategies.

@Nemo157
Copy link
Member

Nemo157 commented Mar 5, 2020

IMO adding a bound to the unbounded queue seems wrong. What might be more reasonable is adding a flag to the bounded queue to make it return SendErrorKind::Full when it hits the bound, instead of Pending.

@Nemo157
Copy link
Member

Nemo157 commented Mar 5, 2020

Alternatively there could be a SendNB future that converts a Pending response from poll_ready into an Error, which would then work with any Sink implementation.

@gnunicorn
Copy link
Author

gnunicorn commented Mar 6, 2020

I see what you mean. This PR comes mostly from the way they are being used in practice. Unbound allows for a fire-and-forget-pattern, thus can be used from outside future-driven-code, while the channel-require you to deal with back-pressure and effectively forces you to also run a futures-poll. Arguably the later is better pattern.

But with the code not being ready to do that, the choice is between having something that potentially eats up all your memory or stalls a thread because the chosen number is too small – and neither case is easy to detect.

The main difference between the bound and unbound for is, then, is that the later never parks. If you say that having a max-buffer on bound is wrong, the other approach I would see to introduce an unbound_send that, similarly to Unbounded, doesn't park but would error if the queue is full. Would that be something acceptable?


Edit: Looking at that a bit more higher level, I could see other usage patterns as well – like dropping the first or the new items when being full, offering ring- or dropping-buffers as overflow pattern alongside the currently default backpressure-pattern and send would depend on that stragtegy. In this scenario the current Unbound-system would just be the generic Receiver/Sender with the UNBOUND-pattern configured and a max buffer of MAX_CAPACITY would just be a regular send (rather unbound_send) and not park.

@gnunicorn
Copy link
Author

This now takes a newer, different approach a I had outlined before. For full-on backwards compatibility it still exposes the channel and unbound by the return the same types, just set up slightly differently: the first has backpressure, the other only ever errors on MAX_CAPACITY. Further you can chose on two more strategies to handle a full queue (to be passed into channel_strategy) dropping either the oldest (Ring) or newly added (Ignore) element added rather than overflowing. This allows for limited non-back-pressuring queues and still backpressure as before.

This removes a lot of (almost duplicate) code and combines the entire system into one common type. Unfortunately the exact strategy checking is a bit divided up in a few places – multiple approaches on having them combined in one place failed because many tests expect the exact order of task parking and wake up to happen in that very specific order (running from the same tasks) or they'd stall forever. While I could have changed the tests, I expect that this might be something other people rely upon and to not break anything downstream went for slightly more complicated code instead.

If this is an approach you consider worth while, I am happy to:

  • update the docs (they are not reflective of what is happening)
  • add tests for the newly added strategies
  • fix CI (mostly backwards compatibility and removing mut where it has become unnecessary to fix warnings).

What do you think?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-channel Area: futures::channel
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants