-
Notifications
You must be signed in to change notification settings - Fork 15
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
Reconsider unpredictable hot/cold and async/sync behavior #170
Comments
I share similar concerns, albeit on much smaller projects. Async vs sync can be annoying, but I feel hot vs cold is even more important. I don't know what kind of API could solve those annoyances, but if there's a reasonable solution, it would be awesome to standardize on that instead. I'd like to hear the thoughts of @benlesh about this, as I am sure that must have come up in the years of maintaining rxjs. |
Thanks so much for filing this. Thoughts below. Async vs sync callbacksI understand and am sympathetic to the async vs sync concern here, but I'm not sure how to handle it without making things more complicated. From reading the supplied articles, it seems to me that the concern is not so much that I really think in the general case — long after
This seems to just move the "this API is both sync and async" concern from
I'd love some more thoughts on this. The simplest thing here is (1) above, but I think it's a dealbreaker for event integration. The other options I've thought of are just as tricky/complicated as the current state of affairs, but have the added downside of breaking the Observable contract that any users today expect, for unclear benefit. Hot vs cold (side effects)I don't think I understand this concern very much, but I could just be missing something big. I'm considering the following comments:
Our proposal here has no "shared" state across subscriptions, so if any call to Maybe the concern is the user code inside the subscriber callback has some "shared" state, where it doesn't perform the side effects more than once (i.e., for the 2nd+ subscription to the same Observable). Is that the concern? If so, I feel like Promise-returning APIs have the same concern. You could imagine some API that (a) does something super expensive, and (b) vends a Promise representing the value. If that API is called many times, you might get a Promise that just replays the cached shared value, as to not re-kick-off the expensive operation. That "tricky"/optimized user code is the same as the user code that would be in the subscriber callback, that's deciding to share/replay some state across many subscriptions to the same Observable. |
It's funny, I actually spoke directly to @isaacs (the author of the Zalgo blog article) about this a long time ago (probably 2015) when I ran into him at a birthday party for a mutual friend in the Bay Area. "Zalgo" can be problematic for sure, but it's really not appropriate to worry about that for a reactive programming type like this. Observables are fairly low level. More importantly though, this needs to work for EventTarget, and if it forces asynchrony (like a promise does) it doesn't work because of things like form.when('submit').subscribe(e => {
// If this is always async, it's too late.
e.preventDefault();
});
element.when('click').subscribe(e => {
// If this is always async, it's too late.
e.preventDefault();
}) Further, const handler = () => console.log('event handled!');
console.log('adding listener');
eventTarget.addEventListener('test', handler);
console.log('dispatching');
eventTarget.dispatchEvent(new CustomEvent('test'));
console.log('removing listener');
eventTarget.removeEventListener('test', handler);
// logs
"adding listener"
"dispatching"
"event handled!"
"removing listener" Where this should do the same: const ac = new AbortController();
console.log('adding listener');
eventTarget.when('test').subscribe(
() => console.log('event handled!'),
{ signal: ac.signal },
);
console.log('dispatching');
eventTarget.dispatchEvent(new CustomEvent('test'));
console.log('removing listener');
ac.abort(); |
On the front of "hot" and "cold": Really what you're talking about is forced multicast. That comes with complications and overhead.
It's plausible to make everything "hot" or "multicast" by default... however here are the complications:
On the other hand, the "cold" observable, which is the most basic, low-level design for observable, is almost a fancy function with some guarantees. I can be used to compose any of the behaviors above quite easily. Another thought: Most of the desire around forced multicast for observables generally comes from people wanting to use observables for state management. While observables can be used for state management, really, they're best at composing events. Something like Signals are better used for state management. |
I don't understand what you mean by If someone passes a Promise into a function there's nothing I can do to that Promise to end up back in sync land. Bluebird did actually have that behavior back in the day and it was quite painful and thankfully native Promises didn't carry it over. An example fix for this would be for the the subscribe callback to emit on microtask, which means even if the subscription is created synchronously the callback is always async which avoids Zalgo. @benlesh Events emitted by the browser will always have a microtask checkpoint before going back into native code, which means even if there's a microtask resolve you could preventDefault(), so your examples do work. const a = document.body.appendChild(document.createElement('a'))
a.textContent = "Click me!";
a.href = "https://www.google.com/";
a.onclick = (e) => {
Promise.resolve().then(() => e.preventDefault());
}; The situation that doesn't work is manually dispatched events (calling dispatchEvent/click from inside JS). That's a platform issue where there's no way to manually trigger a microtask checkpoint and APIs behave differently when done from the JS vs native code. The fix for that is to either expose manual checkpoints or add a |
Please see the sentence immediately preceding the one you quoted:
In other words, I'm describing (with the sentence " |
TIL, @esprehn, thanks! That's good news. However there are still issues with scheduling for every emitted value.
An example for problem number 2 above: function* allIntegers() {
let i = 0;
while (true) yield i++;
}
const ac = new AbortController();
Observable.from(allIntegers()).subscribe(n => {
console.log(n);
if (n === 10) {
ac.abort();
}
}, { signal: ac.signal }); Which seems like nonsense, sure, it's just taken to the extreme... but someone could do something with it that runs through a potentially large set of data, but you only really want to get the first few that match something then run it. window.when('message')
.switchMap(e => {
return Observable.from(e.data.rows)
.filter(row => row.shouldStream)
.take(3)
.flatMap(row => streamRowData(row))
})
.subscribe(updateView, { signal }) Now... you could ABSOLUTELY just do the filtering and taking in the iteration part of this code. BUT observable is the dual of iterable... it should "just work" in roughly the same way no matter which type you choose. If observable schedules, however, it cannot. The only other argument I've seen (and it's even an argument I've made myself) is that "observable should schedule because we don't get the cancellation mechanism back until after the And again... I'll go back to say that for more than a decade of very broad use, observable has not needed scheduled emissions. None of the implementations we cite in the README as examples of prior art add any additional scheduling. I would expect to find new issues if we introduced this behavior, as it's a breaking change from what has been used for so long. Different unknowns and edge cases. |
I disagree with this statement. It's not been my experience, which is why I filed this bug. I oversee a massive rxjs codebase maintained by thousands of engineers. Taking a step back, I don't think "the sync firehose" makes sense on the web platform. Can you give a real example? The unbounded buffer is blocking the thread which is not something you should be doing on the web platform since it prevents promises from resolving and the thread from being responsive to user input. If you need an actual unbounded firehose that sounds like you should use an async iterator inside the callback which would create microtasks interleaving consumption and emission creating those opportunities to unsubscribe. Taking your example: window.when('message')
.switchMap(e => {
return Observable.from(AsyncIterable.from(e.data.rows))
.filter(row => row.shouldStream)
.take(3)
.flatMap(row => streamRowData(row))
})
.subscribe(updateView, { signal }) does exactly what you want. See also: https://github.com/tc39/proposal-async-iterator-helpers |
This is a magic recipe though. Forced asynchrony adds unnecessary complexity and overhead to the type. It introduces a lot of unknowns and questions and untested behaviors that will challenge the entire API, and I have yet to see why that would be beneficial beyond worries about "Zalgo" which aren't really appropriate for this type. In a world with this brand new, never-been-tried forced async observable type, what happens here? const source = new Observable((subscriber) => {
let n = 0;
while (subscriber.active) {
subscriber.next(n++);
}
});
source.take(10).subscribe(console.log); In the untried new observable of forced async, it blocks the thread. With a synchronous observable, it does not. And what happens here? Are the async task logs interleaved with the map calls and the subscribe log? Do we need to queue the value and schedule the processing of of every projected map value? button.when('click')
.map(() => {
console.log('in map 1')
queueMicrotask(() => {
console.log('do something async');
});
return Date.now()
})
.map(() => {
console.log('in map 2')
queueMicrotask(() => {
console.log('do something async');
});
return Date.now()
})
.subscribe(console.log) There's just so many unknowns to this newly proposed type. The scheduling allocations per turn alone would scale terribly, and I'm not even sure there's a mechanism for cancelling a micro task yet. Even if we tried to be conservative about this change and said "Well, let's only schedule the subscription, not the emissions" so we prevent all of the overhead and other weird issues with cancellation, Then there's the question about subscriptions inside of methods. If you have Forcing asynchrony would be a huge mistake and will actually make things harder to reason about, not easier. And at the cost of additional memory footprint and processing time. |
I'm totally willing to die on this hill, honestly. Even the title of this thread unfairly characterizes the behavior of this very basic type as "unpredictable", when it's in fact completely predictable until you force it to schedule with other arbitrary microtasks. |
Thinking about this even more. If there was forced scheduling, testing observables becomes even more cumbersome. it('should handle events via an observable', async () => {
const target = new EventTarget();
const results = [];
target.when('test')
.map(e => e.detail.value)
.subscribe((value) => {
results.push(value);
});
// wait for subscription to start.
await Promise.resolve();
target.dispatchEvent(new CustomEvent('test', { detail: 'hi' }));
// wait for event to propagate
await Promise.resolve();
expect(results).toEqual(['hi']);
}); ...which is even less ergonomic than Even if you use some harness to "click" a button, you'll have to wait a tick to know for sure the click happened. It's just more complicated. |
Now.. all of this said, if you really wanted to consume an observable with forced asynchrony, it implements |
After the discussion the other day, I tossed together a "hot observable", for sake of helping me think through it, and it predictably raises questions:
|
I feel like promises give a precedent for all these questions?
|
This would mean that If the observable is cold, the developer can control when the underlying event listener is added or removed: const clicks = button.when('click');
const ac = new AbortController();
// Add the event listener
clicks.subscribe(console.log, { signal: ac.signal });
// remove the event listener
ac.abort(); Otherwise, with "instantly hot" observables, we'd have to be able to pass an const ac = new AbortController();
button.when('click', { signal: ac.signal })
.filter(e => e.clientX > 100, { signal: ac.signal })
.subscribe(console.log, { signal: ac.signal });
ac.abort(); Because the result of
When composing events with observables, it's often desirable to use methods like const ac = new AbortController();
moveableElement.when('mousedown').switchMap(() => {
return document.when('mousemove')
.takeUntil(document.when('mouseup'))
.map(e => {
return { x: e.clientX, y: e.clientY }
})
})
.subscribe(({ x, y }) => {
moveableElement.style.left = `${x}px`;
moveableElement.style.top = `${y}px`;
}, { signal: ac.signal }) In the above example, It's fixable, of course, but only if you declare the other observables at the top level (which interestingly, still works with "cold" observables): const ac = new AbortController();
const documentMouseMoves = document.when('mousemove');
const documentMouseUps = document.when('mouseup');
moveableElement.when('mousedown').switchMap(() => {
return documentMouseMoves
.takeUntil(documentMouseUps)
.map(e => {
return { x: e.clientX, y: e.clientY }
})
})
.subscribe(({ x, y }) => {
moveableElement.style.left = `${x}px`;
moveableElement.style.top = `${y}px`;
}, { signal: ac.signal }) Which, of course, doesn't account for my previously mentioned issue with needing a way to pass an |
Something to take into account with Also, I think we don't have to think of observables as promises, they're more like streams in a way. Attaching a reader/pipe etc to a stream might invoke the write method on the other side and create a chain reaction. I think that's a closer precedence. |
I've started a new discussion specifically about the idea of ref-counted observables here: #178 (comment) |
I thought again about the scroll/touch issue (adding event listeners has performance implications). What if we made it so that creating observers for these events is passive by default, and if you want the active behavior (preventing default behavior etc) you have to use regular event listeners? This feels aligned with the overall purpose of observers where they're less tuned towards overriding platform behavior and more towards, well, observing it (passively as data). If we had this behavior for event listeners, and as a guideline that in general the observer registration should not be expensive, perhaps registering a listener with each This also leads me to think that this question is not at all about observables as a general construct, but about the observable shim with the platform, e.g. |
First off, I think setting https://wicg.github.io/observable/#ref-for-event-listener-passive to true by default is probably a good idea, assuming we'd want to do that with Second, I think that's all separate from the issue of Observables having a ref-counted producer and being cold/lazy. We are not making Observable producers ref-counted for performance reasons a la passive listening nearly as much as we're doing it for predictability upon multiple subscriptions.
The idea of passive event listening doesn't apply outside of Observables returned by |
Oh I wasn't aware of being able to pass
Alrighty. |
Off the top of my head, I feel like the most common case for this is form submit events. Where the developer wants to handle the form submissions manually with fetch or over a socket or something. They need to prevent the default behavior. "Cancellable" in this case, would mean someone wants to unmount a component containing a form, but still have a subscription to the observable they got from |
Yeah I'm not sure we should invert |
The rxjs inspired API creates a situation where Observables sometimes have side effects upon subscription (hot vs cold) and sometimes emit synchronously and sometimes async. If you have a function that takes an Observable as an argument you have no predictability if it will have a side effect upon subscription (is it safe to call it twice?), and no predictability if the emit will happen immediately when subscribing or later in a microtask.
This is a manifestation of Releasing Zalgo:
https://blog.izs.me/2013/08/designing-apis-for-asynchrony/
https://blog.ometer.com/2011/07/24/callbacks-synchronous-and-asynchronous/
Promises are the opposite of this:
This means if someone passes a Promise to you, all operations on it are predictable and safe.
With Observables the situation is different:
In large systems built on Observables I see this manifest often as:
Other Web APIs are also consistent today (ex. MutationObserver, ResizeObserver, Promises), and never emit during subscription and have a split between observing the operation and executing it. The one place I'm aware where this was violated is MessagePort#onmessage starting the port, but it's generally been considered a mistake.
Context: I lead possibly the largest codebase built on rxjs in the world.
The text was updated successfully, but these errors were encountered: