1.x: make scan's delayed Producer independent of event serialization #3491
Conversation
It turns out serializing `request()` calls with regular `onXXX()` calls can be problematic because a `request()` may trigger an emission of events which then end up being queued (since `emitting == true`). If the request is large and the queue otherwise unbounded, this will likely cause OOME. In case of `scan`, the fix was to make the missing request accounting and arrival of the `Producer` independent of the event's emitter loop; there is no need for them to be serialized in respect to each other. In case of the `ProducerObserverArbiter` where the request accounting and producer swapping has to be serialized with the value emission, the solution is to call `request()` outside the emitter-loop.
|
Can we get this one reviewed and in a release soon please? This is a blocker for me and precludes me using 1.0.15 and 1.0.16. It's a serious bug with |
| @@ -270,23 +235,51 @@ public void onCompleted() { | |||
| emit(); | |||
| } | |||
|
|
|||
| @Override | |||
| public void request(long n) { | |||
There was a problem hiding this comment.
@akarnokd in the future if its possible to do so could you not rearrange the methods? It makes it so much easier to to read the changes to request side by side instead of seeing one section of code missing entirely only to reappear (modified) later on. Thank you, it's a suggestion to help expedite the PR review process.
There was a problem hiding this comment.
I did this move out of way so the interleaved view doesn't confuse the reviewer since the original code was simply wrong. This is rarely happening so other PRs will have changes in place.
There was a problem hiding this comment.
Okay thanks for your consideration. As I said it will help a lot for me to be able to see changes interleaved.
|
Is |
|
It was added as a tool for building operators but I usually inline the algorithm. |
|
Okay. So it's more like a pattern that gets pasted into operators. Do you usually inline for performance reasons or for custom functionality? |
|
👍 |
1.x: make scan's delayed Producer independent of event serialization
It turns out serializing
request()calls with regularonXXX()calls can be problematic because arequest()may trigger an emission of events which then end up being queued (sinceemitting == true). If the request is large and the queue otherwise unbounded, this will likely cause OOME.In case of
scan, the fix was to make the missing request accounting and arrival of theProducerindependent of the event's emitter loop; there is no need for them to be serialized in respect to each other.In case of the
ProducerObserverArbiterwhere the request accounting and producer swapping has to be serialized with the value emission, the solution is to callrequest()outside the emitter-loop.There shouldn't be any issue with 2.x
scan()because in 2.x, scan receives theSubscriptionbefore it allows the downstream to request anything so there is no missing requested to be handled.This should resolve #3490. As far as I can remember, no other operator should have such problems because all others use
ProducerArbiterwhich is independent ofonXXXemission serializations.