Fix driver race condition on sinks, fix DOM driver mutations in loop #719

staltz · 2017-10-17T13:58:15Z

There exists an issue discussing the need for this PR, HTTP driver erroneously replays previous request #592 and DOM driver elements() sync emissions in a loop #699
I added new tests for the issue I fixed or built
I ran make test FOO for the package FOO I'm modifying
I used make commit instead of git commit

jvanbruegge · 2017-10-17T14:10:45Z

~~Note that this will drop IE10 and IE9 support (http://caniuse.com/mutationobserver/embed)~~
There is a polyfill for IE9+

staltz · 2017-10-17T14:21:03Z

@jvanbruegge Yes, I was aware but forgot to mention it. We already don't support IE9, but IE10 can support this through a polyfill

jvanbruegge · 2017-10-17T14:23:05Z

What kills IE9?

staltz · 2017-10-17T14:54:14Z

@jvanbruegge There's an issue about it, still open. (I'm now on mobile, harder to search)

staltz · 2017-10-17T15:51:29Z

It was this issue: #224

staltz · 2017-10-17T18:52:07Z

To-do according to CI failures:

? ~~Add a polyfill when testing IE10~~
Test cycle/http with the newest cycle/run rc
Investigate cycle/dom test failing in Chrome v49 (didn't happen anymore)

staltz · 2017-10-26T11:12:05Z

I'm considering dropping official support for IE10. I added a MutationObserver polyfill when testing IE10, and most tests passed, but tests related to DocumentFragment didn't pass. I think the MutationObserver polyfill can't detect that a DocumentFragment was mutated because fragments are by definition "detached" from the normal document, maybe that's why it can't detect mutations.

DocumentFragments and Cycle.js in IE10 would only be used for Web Components (I think), so I think it's fair to not support Cycle.js + Web Components in IE10, but normal Cycle.js DOM usage (without Web Components) would still work.

This changes the internal workings of Cycle Run so that sink proxies are normal xstream streams, not MemoryStreams. BREAKING CHANGE: This is a breaking change that requires updating your project to use the latest Cycle Run as well as the latest Cycle HTTP and Cycle DOM all at the same time, otherwise you may see race conditions across drivers. ISSUES CLOSED: #592

Every sink stream emission from the main() is scheduled on the microtask available (MutationObserver or process.nextTick or setTimeout) in order to give enough time for subscriptions to happen in main() and avoid race conditions such as issue #592. BREAKING CHANGE: This means Cycle run() function is no longer synchronous, so if for some reason you were depending on the synchronicity of run(), you will lose this guarantee. This is breaking change, but rarely should this be experienced. With the new microtask scheduling, sinks are emitted to drivers as soon as the current event loop scripts are completed, so there will not be noticeable delays in the user interface (like setTimeout would incur). ISSUES CLOSED: #592

BREAKING CHANGE: Like Cycle run() v4, RxJS run() will not anymore execute synchronously once the run() function is called, instead it will schedule sink emissions as microtasks that happen as soon as the current event loop scripts are completed.

Frikki · 2017-10-26T21:10:10Z

dom/src/makeDOMDriver.ts

+    // exist before elementAfterPatch$ calls mutationObserver.observe()
+    mutationConfirmed$.addListener({});
+
+    const elementAfterPatch$ = firstRoot$


This is quite convoluted, and I'm sure this could be resolved with adding a scheduler to xtream.

What is convoluted about this?
I believe scheduling is a solution to #592, but not for #699 (which this code right here is about), because #699 needs to be solved by somehow detecting if the DOM was mutated or not.

Convoluted: merge -> map -> fold -> drop -> map -> startWith -> map -> compose, each with various degrees of complexity. It is hard to reason about. Smaller steps where the outputs of the various operations are assigned to well-named variables makes it much easier for anyone who didn't write this code to understand it. Either that or apply some comments that explain the why.

I believe scheduling is a solution to #592, but not for #699 (which this code right here is about)

Fair enough.

Sure, it's a long chain of operators. And yes, one can easily refactor that by just splitting it up and placing them in convenient helper functions. Has nothing to do with schedulers, though.

Good that I understood your comment is about code style, not about the actual fix.

The scheduler mention is related to this PR, as the PR references #592 explicitly. It may not have a direct impact on this patch solution, but indirectly it definitely has. IMO, it is an underlying issue with xstream. For example, Most.js doesn’t emit during Stream.run, and I think that the same behavior applied to xstream would be the solution to all these issues. Just my two cents.

Of course, if this is just a temporary patch that will be addressed again later when #592 is handled, then I understand the approach.

@Frikki, most.js also has scheduling problems: cujojs/most#414
And adding a trampoling scheduler (like RxJS 4 had) has its downsides too. This PR has a comprehensive solution without introducing a scheduler.

To that end, I do not agree with the solution. As you know, we are working intensively with Motorcycle and @most/core. We do not have these issues. But it’s fine if Cycle.js and xstream want things implemented differently. After all, Cycle.js and Motorcycle are not the same, though they deal with the same concept. Thanks for the feedback.

When vnode$ depends on element$ and vice-versa, a synchronous infinite loop might occur, which is issue #699. This commit solves that by using MutationObserver, used to indicate when to emit a new element on element$. BREAKING CHANGE: Internet Explorer 10 is no longer officially supported, but it can still be used with cycle/dom under some circumstances. You should use a polyfill for MutationObserver and make sure you are not rendering the application in a DocumentFragment as the container node. Only under those conditions will cycle/dom should work correctly in IE10. ISSUES CLOSED: #699

staltz · 2017-10-31T19:36:10Z

Travis CI tests pass.

@jvanbruegge could you give a final review?

staltz · 2017-11-09T10:43:18Z

I hope someone can take a look at this before merging, because it has some important breaking changes and fixes. I know it's a big PR but it's easier to review commit by commit. There are only 3 important commits actually:

Replication sinks are Streams, not MemoryStreams 8d50824
Sink emissions are scheduled as microtasks 1c1d23d
DOM Driver uses MutationObserver 8cffe90

ntilwalli · 2017-11-09T16:19:38Z

This looks good to me too. I just experienced this issue again in a new context. Onion state$ emits an old state in some cases value when there is a circular dependence. Fixed by introducing a delay

staltz · 2017-11-10T08:33:40Z

Thanks @ntilwalli and @jvanbruegge !

staltz added the PR: awaiting review label Oct 17, 2017

This was referenced Oct 17, 2017

DOM driver elements() sync emissions in a loop #699

Closed

Driver buffers to solve race conditions from issue #592 #658

Closed

staltz added PR: work in progress and removed PR: awaiting review labels Oct 17, 2017

staltz force-pushed the issue592-nexttick branch from fb60545 to 1990864 Compare October 17, 2017 19:34

staltz self-assigned this Oct 17, 2017

staltz force-pushed the issue592-nexttick branch 5 times, most recently from 1475ca5 to 05172a3 Compare October 25, 2017 17:00

staltz force-pushed the master branch 2 times, most recently from 19355d6 to 61f81cf Compare October 26, 2017 07:54

staltz force-pushed the issue592-nexttick branch from 05172a3 to ce31173 Compare October 26, 2017 09:52

staltz added 7 commits October 26, 2017 15:39

test(dom): increase timeouts for some tests, for reliability

08160c6

test(http): increase timeouts for some tests, for reliability

e4f31c5

refactor(run): release candidate v4.0.0-rc.3

ba51ebd

fix(rxjs-run): depend on run v4

34b204e

BREAKING CHANGE: Like Cycle run() v4, RxJS run() will not anymore execute synchronously once the run() function is called, instead it will schedule sink emissions as microtasks that happen as soon as the current event loop scripts are completed.

refactor(rxjs-run): release candidate v8.0.0-rc.3

fc99090

staltz added the PR: work in progress label Oct 26, 2017

Frikki reviewed Oct 26, 2017

View reviewed changes

staltz added 4 commits October 31, 2017 18:53

refactor(run): release candidate v4.0.0-rc.4

f2662eb

refactor(rxjs-run): release candidate v8.0.0-rc.4

45e3ca5

test(http): update tests, using cycle/run v4.0.0-rc.4

7d21034

staltz force-pushed the issue592-nexttick branch from baf1a95 to 6004260 Compare October 31, 2017 16:55

staltz closed this Oct 31, 2017

staltz reopened this Oct 31, 2017

test(dom): add polyfills for IE10 when testing

28243b7

staltz force-pushed the issue592-nexttick branch from 6004260 to 28243b7 Compare October 31, 2017 18:03

staltz added PR: 1 LGTM and removed PR: work in progress labels Oct 31, 2017

refactor(dom): release candidate v20.0.0-rc.1

0e3c151

staltz closed this Nov 9, 2017

staltz reopened this Nov 9, 2017

jvanbruegge added PR: 2 LGTM and removed PR: 1 LGTM PR: awaiting review labels Nov 9, 2017

staltz closed this Nov 9, 2017

staltz reopened this Nov 9, 2017

staltz merged commit aec2825 into master Nov 10, 2017

staltz deleted the issue592-nexttick branch November 10, 2017 08:34

geovanisouza92 mentioned this pull request Jan 28, 2018

Sync/Async compatibility with promises & callbacks tc39/proposal-observable#179

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix driver race condition on sinks, fix DOM driver mutations in loop #719

Fix driver race condition on sinks, fix DOM driver mutations in loop #719

staltz commented Oct 17, 2017

jvanbruegge commented Oct 17, 2017 •

edited

Loading

staltz commented Oct 17, 2017

jvanbruegge commented Oct 17, 2017

staltz commented Oct 17, 2017

staltz commented Oct 17, 2017

staltz commented Oct 17, 2017 •

edited

Loading

staltz commented Oct 26, 2017

Frikki Oct 26, 2017

staltz Oct 27, 2017

Frikki Oct 27, 2017

staltz Oct 27, 2017

Frikki Oct 28, 2017 •

edited

Loading

staltz Oct 28, 2017

Frikki Oct 29, 2017

staltz commented Oct 31, 2017

staltz commented Nov 9, 2017

ntilwalli commented Nov 9, 2017 •

edited

Loading

staltz commented Nov 10, 2017

Fix driver race condition on sinks, fix DOM driver mutations in loop #719

Fix driver race condition on sinks, fix DOM driver mutations in loop #719

Conversation

staltz commented Oct 17, 2017

jvanbruegge commented Oct 17, 2017 • edited Loading

staltz commented Oct 17, 2017

jvanbruegge commented Oct 17, 2017

staltz commented Oct 17, 2017

staltz commented Oct 17, 2017

staltz commented Oct 17, 2017 • edited Loading

staltz commented Oct 26, 2017

Frikki Oct 26, 2017

Choose a reason for hiding this comment

staltz Oct 27, 2017

Choose a reason for hiding this comment

Frikki Oct 27, 2017

Choose a reason for hiding this comment

staltz Oct 27, 2017

Choose a reason for hiding this comment

Frikki Oct 28, 2017 • edited Loading

Choose a reason for hiding this comment

staltz Oct 28, 2017

Choose a reason for hiding this comment

Frikki Oct 29, 2017

Choose a reason for hiding this comment

staltz commented Oct 31, 2017

staltz commented Nov 9, 2017

ntilwalli commented Nov 9, 2017 • edited Loading

staltz commented Nov 10, 2017

jvanbruegge commented Oct 17, 2017 •

edited

Loading

staltz commented Oct 17, 2017 •

edited

Loading

Frikki Oct 28, 2017 •

edited

Loading

ntilwalli commented Nov 9, 2017 •

edited

Loading