davidMcneil / rants

An async NATS client library for the Rust programming language.
Apache License 2.0
81 stars 11 forks source link

Implement backpressure in subscriber #2

Closed semtexzv closed 5 years ago

semtexzv commented 5 years ago

This change affects only a single situtation. When the channel is full, the previous implementation just threw its hands into the air, and waved "Screw you".

After this change, it is much more well behaved. If the channel is full, it now wakes up the receiving task, forcing it to handle messages already in the channel, and sleeps until this is performed. Which is effectively implementing backpressure. Note : The behavior when channel is not full is unchanged, The client will push messages into it, and they get handled only after the client has yielded the control back to the executor.

But it means that client can now be blocked by slow receiver. But I think this is much better default behavior than dropping messages. It might be beneficial to provide both strategies.

davidMcneil commented 5 years ago

Thank you for the PR! This is a very good change. As you said, the client being blocked by a slow receiver was my concern with this approach. Looking at the problem again, I think a good solution would be to spawn a future instead of awaiting handle_server_message. This has the additional benefit of cleaning up the code a bit because we can remove all spawns in the handle_server_message method.

I believe this fixes the backpressure issue as well as preventing us from being blocked by a slow receiver. However, if we have a slow receiver we could spawn an unbounded number of futures, but I think that is ok for now and if it is a problem someone runs into we could implement a way to specify what strategy to use.

What do you think? Would you mind implementing this?

semtexzv commented 5 years ago

I'd rather allow user to specify an unbounded Channel size when subscribing. This would allow both backpressure and no backpressure, and would not complicate the library that much.

davidMcneil commented 5 years ago

Yeah, you are correct that is a better approach. If a future was spawned every time, it would make every channel look like it was unbounded which would never allow for backpressure. Thanks again for the PR.