#235 flatMapLatest adding overlapping option #236
Conversation
passing an options object of {overlapping:true} will cause the next stream to be added before any old streams are removed.
src/many-sources/abstract-pool.js
Outdated
| this._addToCur(toObs(obj)); | ||
| while (this._curSources.length > this._concurLim) { | ||
| this._removeOldest(); | ||
| } |
There was a problem hiding this comment.
this loop seemed a little weird when I was working on it, however, I think to have correct parity with the other side of the condition it needs to be here. Given that this._curSources.length - this._concurLim > 1, the other side, implicitly loops due to recursion. Since we want to add first, using _addToCur, then we need to make sure that we aren't over the _concurLim and blindly removing one might be not enough. Although, I'm not sure how an AbstractPool could get into that state.
There was a problem hiding this comment.
I think we should just remove one. Yes, there is a recursion on the other side, but it should always do only one iteration because:
- We always expect this condition to be true:
this._curSources.length <= this._concurLim. - When we get to this branch of the code
this._curSources.length === this._concurLimshould be true. - We remove one from
this._curSources, so nowthis._curSources.length === this._concurLim - 1. - At the following
this._add()call, we fall into branch at line 33. And that is the end.
But more importantly, with the condition you added we might actually remove none of the oldest. After this._addToCur(toObs(obj)) we not necessarily have an observable added to _curSources because if toObs(obj) already anded or ends immediately after we subscribe to it, we don't add it to the _curSources (take a look at _addToCur). Btw, we need a test case for this.
There was a problem hiding this comment.
Also maybe it make sense to remove recursion on the other side too.
this._removeOldest();
- this._add(obj, toObs);
+ this._addToCur(toObs(obj));This way we get the correct parity.
There was a problem hiding this comment.
I'm not sure thats quite equivalent; there may not be a public api to aggravate the condition, but I think that when { concurLim: 1, queueLim: 1, drop: 'old' }, and given that both _curSources and _queue are full and I add another one, I would want to:
- drop the eldest item out of
_curSources - promote the eldest item from
_queue - add_ the new_ stream to the
_queue
But, I'll need to think about it more. However, I think the same concern is valid for the overlapping side of the branch. If there is a queue, then one should add to _queue and let _removeOldest pull it out of the queue, but that doesn't really resolve the actual use-case for using overlapping to begin with. I think I need to step back and think about it harder.
There was a problem hiding this comment.
Ah, right! I didn't realize that _removeOldest pulls from the queue. Replacing this._add -> this._addToCur isn't equivalent then indeed. I'll think about it too.
There was a problem hiding this comment.
I haven't test it, but maybe we could do something like this?
_add(obj, toObs /* Function | falsey */, allowOverflow) {
toObs = toObs || id;
if (this._concurLim === -1 || this._curSources.length < this._concurLim) {
this._addToCur(toObs(obj));
} else {
if (this._queueLim === -1 || this._queue.length < this._queueLim || allowOverflow) {
this._addToQueue(toObs(obj));
} else if (this._drop === 'old') {
if (this._overlapping) {
this._add(obj, toObs, true);
this._removeOldest();
} else {
this._removeOldest();
this._add(obj, toObs);
}
}
}
}
|
Thank you for the PR! I'll take a look tomorrow. And sorry for the delays, quite busy at work. |
src/many-sources/abstract-pool.js
Outdated
| } else { | ||
| this._removeOldest(); | ||
| this._add(obj, toObs); | ||
| this._addToCur(toObs(obj)); |
There was a problem hiding this comment.
while cleaner, this doesn't feel right. see my previous comment, I need to think more about this/this is more subtle than I initially projected.
|
This would be a nice-to-have. Anything blocking? |
|
@32bitkid Would love to land this, if possible. There's been some changes to our test suite in particular (it's in JS now woo!), so the |
|
@mAAdhaTTah There are also some unresolved concerns in folded comments. |
|
@mAAdhaTTah This is a feature that I'd really like to get in as well, but have been swamped with work lately. As @rpominov mentioned, there are some unresolved issues that still need to be addressed/reworked. I'll try to spend some time this weekend on this. |
|
This PR is now 4 years old, I've worked on a patch to be applied on the current codebase, and I wonder if I should open a PR with it. What do you think? |
|
@bpinto Can't hurt to open a PR! |
|
I'll go through the comments on this PR again and see what I can do, I believe I have a better understanding of the feature than I had when I wrote the patch. I'll open a PR then. |
This will cause the next stream to be added before any old streams are
removed.
```
const channelA = Kefir.stream((emitter) => {
console.log('connect a');
let count = 0, id = setInterval(() => emitter.value(count++), 250);
return () => { console.log('disconnect a'); clearInterval(id); };
});
const channelB = Kefir.stream((emitter) => {
console.log('connect b');
let count = 0, id = setInterval(() => emitter.value(count++), 250);
return () => { console.log('disconnect b'); clearInterval(id); };
});
const data = {
a: channelA,
b: Kefir.combine([channelA, channelB]),
c: channelB,
};
Kefir.sequentially(1000, ['a', 'b', 'c', undefined])
.flatMapLatest(p => p ? data[p] : Kefir.never())
.log('result');
```
With overlapping option disabled (default):
```
> connect a
> result <value> 0
> result <value> 1
> result <value> 2
> disconnect a
> connect a
> connect b
> result <value> [0, 0]
> result <value> [1, 0]
> result <value> [1, 1]
> result <value> [2, 1]
> result <value> [2, 2]
> disconnect a
> disconnect b
> connect b
> result <value> 0
> result <value> 1
> result <value> 2
> disconnect b
> result <end>
```
With overlapping option enabled:
```
> connect a
> result <value> 0
> result <value> 1
> result <value> 2
> connect b
> result <value> [3, 0]
> result <value> [3, 1]
> result <value> [4, 1]
> result <value> [4, 2]
> disconnect a
> result <value> 3
> result <value> 4
> result <value> 5
> disconnect b
> result <end>
```
Closes: kefirjs#235 kefirjs#236
This will cause the next stream to be added before any old streams are
removed.
```
const channelA = Kefir.stream((emitter) => {
console.log('connect a');
let count = 0, id = setInterval(() => emitter.value(count++), 250);
return () => { console.log('disconnect a'); clearInterval(id); };
});
const channelB = Kefir.stream((emitter) => {
console.log('connect b');
let count = 0, id = setInterval(() => emitter.value(count++), 250);
return () => { console.log('disconnect b'); clearInterval(id); };
});
const data = {
a: channelA,
b: Kefir.combine([channelA, channelB]),
c: channelB,
};
Kefir.sequentially(1000, ['a', 'b', 'c', undefined])
.flatMapLatest(p => p ? data[p] : Kefir.never())
.log('result');
```
With overlapping option disabled (default):
```
> connect a
> result <value> 0
> result <value> 1
> result <value> 2
> disconnect a
> connect a
> connect b
> result <value> [0, 0]
> result <value> [1, 0]
> result <value> [1, 1]
> result <value> [2, 1]
> result <value> [2, 2]
> disconnect a
> disconnect b
> connect b
> result <value> 0
> result <value> 1
> result <value> 2
> disconnect b
> result <end>
```
With overlapping option enabled:
```
> connect a
> result <value> 0
> result <value> 1
> result <value> 2
> connect b
> result <value> [3, 0]
> result <value> [3, 1]
> result <value> [4, 1]
> result <value> [4, 2]
> disconnect a
> result <value> 3
> result <value> 4
> result <value> 5
> disconnect b
> result <end>
```
Closes: kefirjs#235 kefirjs#236
passing an options object of {overlapping:true} will cause the next stream to be added before any old streams are removed.