fix: resolve Preboot race conditions #83

mgol · 2018-04-04T15:09:19Z

Please check if the PR fulfills these requirements

The commit message follows our guidelines: https://github.com/angular/preboot/blob/master/CONTRIBUTING.md#commit-message-format
Tests for the changes have been added (for bug fixes / features)
Docs have been added / updated (for bug fixes / features)

What modules are related to this pull-request

What kind of change does this PR introduce? (Bug fix, feature, docs update, ...)
remove unused proxy imports from some test files

A bug fix via a refactor.

What is the current behavior? (You can also link to an open issue here)

See #82.

What is the new behavior (if this is a feature change)?

There should be no race conditions.

Does this PR introduce a breaking change? (What changes might users need to make in their application due to this PR?)

Yes, at least in the inline code handling as it no longer waits for the app root to be available so it cannot be fully put in <head>.

Other information:

Currently Preboot suffers from various race conditions:

It waits for the document.body to be available and then applies its logic.
However, the application root may not be available at this point - see the issue
add condition to check that the application root is available in the DOM before prebooting #72.
Even if the application root exists when Preboot starts listening for events
the server node may not be available in full yet. Since Preboot searches for
nodes matching desired selectors inside of the existing serverNode, it may
skip some of them if the server node hasn't loaded fully. This is especially
common if the internet connection is slow.
Preboot waits for body in a tight loop, checking every 10 milliseconds.
This starves the browser but may also be clamped by it to a larger delay
(around 1 second); that's what happens in modern browsers if the tab where the
page loads is inactive which is what happens if you open a link in a new tab in
Chrome or Firefox. This means Preboot may start listening for events after
Angular reports the app is stable and the PrebootComplete event fires. This
then leaves the server node active, never transferring to the client one which
makes for a broken site.

To solve it, we're doing the following:

Since we want to attach event listeners as soon as possible when the server
node starts being available (so that it can be shallow-cloned) we cannot wait
for it to be available in full. Therefore, we switched to delegated event
handlers on each of the app roots instead of directly on specific nodes.
As we support multiple app roots, to not duplicate large inline preboot code,
we've split getInlinePrebootCode into two parts: function definitions to be
put in <head> and the invocation separate for each app root put just after
the app root opening tag.
To maintain children offset numbers (otherwise selectors won't match between
the client & the server) we've removed the in-app-root script immediately when
it starts executing; this won't stop the execution. document.currentScript is
a good way to find the currently running script but it doesn't work in IE. A
fallback behavior is applied to IE that leverages the fact that start scripts
are invoked synchronously so the current script is the last one currently in
the DOM.
We've removed waitUntilReady as there's no reliable way to wait; we'll
invoke the init code immediately instead.
We've switched the overlay used for freeing the UI to attach to
document.documentElement instead of document.body so that we don't have to
wait for body.
The mentioned overlay is now created for each app root separately to avoid
race conditions.

Fixes #82
Fixes #72

BREAKING CHANGES:

When used in a non-Angular app, the code needs to be updated to use
getInlineDefinition and getInlineInvocation; the previously defined
getInlinePrebootCode has been removed. The getInlineDefinition output
needs to be placed before any content user might interactive with, preferably
in . The getInlineInvocation output needs to be put just after the
opening tag of each app root. Only getInlineDefinition needs to have options
passed. An example expected (simplified) layout in EJS format:

<html>
  <head>
    <script><%= getInlineDefinition(options) %></script>
  </head>
  <body>
    <app1-root>
      <script><%= getInlineInvocation() %></script>
      <h2>App1 header</h2>
      <div>content</div>
    </app1-root>
    <app2-root>
      <script><%= getInlineInvocation() %></script>
      <h2>App2 header</h2>
      <span>content</span>
    </app2-root>
  </body>
</html>

The refactor breaks working with frameworks that replace elements
like AngularJS with UI Router as it no longer uses serverSelector and
clientSelector but expects its clientNode to remain in the DOM.

TODO:

Make it work in IE: add a fallback for missing document.currentScript.
Make sure it works correctly for multiple app roots. ~~Maybe switch the selectors to be scoped to the concrete app root so that they don't interfere with each other?~~ (we're now attaching delegated handlers to the server node, not to document). This may be an issue as we now have separate code for each app root. We need to switch the logic to not use the appRoots array as each of the app roots is now handled separately.
Write some tests? (although the current code is not well tested either so that may require significant time investment)
Decide whether to remove getInlinePrebootCode.

mgol · 2018-04-04T15:19:01Z

README.md

@@ -57,7 +57,8 @@ import { getInlinePrebootCode } from 'preboot';
 const prebootOptions = {};  // see PrebootRecordOptions section below
 const inlineCode = getInlinePrebootCode(prebootOptions);

-// now simply insert the inlineCode into the HEAD section of your server view
+// now simply insert the inlineCode into a `<script>` tag just after


Note to self: maybe remove the word simply; it may be discouraging for people who get stuck at this point.

mgol · 2018-04-04T15:20:35Z

src/lib/api/event.recorder.ts

+  // Remove the current script from the DOM so that child indexes match
+  // between the client & the server. The script is already running so it
+  // doesn't affect it.
+  if (document.currentScript && document.currentScript.parentNode) {


I should take document from win, shouldn't I?

mgol · 2018-04-04T15:22:44Z

src/lib/api/event.recorder.ts

+  eventSelector.events.forEach((eventName: string) => {
+    // get the appropriate handler and add it as an event listener
+    const handler = createListenHandler(_document, prebootData, eventSelector, appData);
+    _document.addEventListener(eventName, handler);


This is the place that I suspect may come in conflict between various app roots as selectors are global. We should perhaps scope them to the current app root by assigning it a unique attribute and checking if event.target in the created event handlers is contained in the proper app root.

I hope we don't have to support nested app roots. ;)

Or maybe just attach the listener to serverRoot instead of _document.

The second one if it works properly would be the way to go

One other thing - we should probably use the capture phase instead of the bubble one. Otherwise if any of the respective event handlers call stopPropagation() it will not get to the server node & the event won't be captured.

mgol · 2018-04-04T15:26:11Z

src/lib/api/inline.preboot.code.ts

+ * @param customOptions PrebootRecordOptions that override the defaults
+ * @returns Generated inline preboot code is returned
+ */
+export function getInlinePrebootCode(customOptions?: PrebootOptions): string {


The issues here are that if someone uses getInlinePrebootCode directly:

They need to move it from <head> to just after the opening app root tag which may not be easy in all scenarios. This is a breaking change.

If multiple app roots are used, each of them will need to get this injected separetely which increases the HTML.

Should we still support this function or just go all the way & require people to use getInlinePrebootCodeDefinition & getInlinePrebootCodeInvocation separately?

Yeah let's cut it. I'll confirm with @jeffwhelpley offline, but so long as we document this change very well, I don't foresee it being a huge problem.

Is there any way we can streamline the process with a utility for React users?

I don't know React well enough so I can't speak for it. How is it used there? AFAIK React has DOM rehydration so there should be no separate server & client node, should it?

Sorry I haven't given comments yet on this PR. I have been slammed getting ready for ng-conf.

So, we did have a decent number of React users before they introduced hydration. It probably isn't as popular any more, but just realize that hydration doesn't solve all the problems. For example, if a user clicks on a button before the client is active, nothing is going to happen.

Regardless, the number of people is likely really small. I suggest we just move forward and we can always add some sugar later for non-Angular users if people complain.

mgol · 2018-04-04T15:28:31Z

src/lib/provider.ts

+          .forEach(appRootElem => {
+            if (appRootElem) {
+              const inlineCodeInvocation = getInlinePrebootCodeInvocation(prebootOpts);
+              const scriptWithInvocation = createScriptFromCode(doc, nonce, inlineCodeInvocation);


In the current version this could be moved out of the loop, near the place where inlineCodeDefinition is created. However, since IE doesn't have document.currentScript and the polyfill for that works only in IE 6-10 we'll need to tag each of those scripts with a unique attribute & inject its value as a variable into the script. Therefore, each of those scripts will be distinct so we'll need to create them in the loop anyway.

The only other solution I see is to trop IE support in Preboot. But I assume that's off the table. ;)

Sounds fine

CaerusKaru · 2018-04-04T16:54:49Z

src/lib/api/event.replayer.ts

@@ -190,7 +190,7 @@ export class EventReplayer {

    // remove the freeze overlay if it exists
    const doc = this.getWindow().document;
-    const prebootOverlay = doc.body.querySelector('#prebootOverlay') as HTMLElement;
+    const prebootOverlay = doc.querySelector('#prebootOverlay') as HTMLElement;


Would getElementById be faster here?

It should be, I'll change it.

CaerusKaru · 2018-04-04T16:55:19Z

src/lib/api/inline.preboot.code.ts

@@ -31,6 +29,8 @@ const eventRecorder = {
  createBuffer
 };

+export const initFunctionName = 'prebootInit';


Maybe a little more specific, prebootInitFn?

mgol · 2018-04-04T21:50:08Z

Another note to self: we still have the logic to handle multiple app roots in the recorder code; it should be changed to only work on one app root. Otherwise we have an O(n^2) complexity where n is the number of app roots, not to mention duplicate event registration.

mgol · 2018-04-05T15:34:20Z

PR updated; I still need to fix IE & refactor Preboot further a little so that it still works fine for multiple app roots but I addressed most comments otherwise.

mgol · 2018-07-25T13:36:59Z

PR rebased & updated; the logic has been rewritten, all it needs is tests & bug reports!

CaerusKaru

LGTM. This is fantastic work. Some very minor nits. Plan is to add tests in a follow-up (and maybe some CI in between)

CaerusKaru · 2018-08-02T14:45:30Z

README.md

-```
-import { getInlinePrebootCode } from 'preboot';
+```ts
+import { getInlinePrebootCodeDefinition, getInlinePrebootCodeInvocation } from 'preboot';


I think PrebootCode makes this too verbose, and getInlineDefinition/getInlineInvocation is enough to avoid collisions with other libs.

CaerusKaru · 2018-08-02T14:52:27Z

src/lib/api/event.recorder.ts

+    [].slice.call(_document.getElementsByTagName('script'), -1)[0];
+
+  if (!currentScript) {
+    console.error('Preboot starting failed, no currentScript has been detected.');


Change starting to initialization

CaerusKaru · 2018-08-02T14:59:41Z

src/lib/api/event.recorder.ts

-  const _document = <Document>(theWindow.document || {});
+  let serverNode = currentScript.parentNode;
+  if (!serverNode) {
+    console.error('Preboot starting failed, the script is detached');


Same note here

CaerusKaru · 2018-08-02T15:02:11Z

src/lib/api/inline.preboot.code.spec.ts

@@ -1,6 +1,6 @@
 import {
-  assign, getEventRecorderCode, getInlinePrebootCode, stringifyWithFunctions,
-  validateOptions
+  assign, getEventRecorderCode, getInlinePrebootCodeDefinition, getInlinePrebootCodeInvocation,


Fix formatting here, one import per line

Currently Preboot suffers from various race conditions: 1. It waits for the `document.body` to be available and then applies its logic. However, the application root may not be available at this point - see the issue angular#72. 2. Even if the application root exists when Preboot starts listening for events the server node may not be available in full yet. Since Preboot searches for nodes matching desired selectors inside of the existing `serverNode`, it may skip some of them if the server node hasn't loaded fully. This is especially common if the internet connection is slow. 3. Preboot waits for `body` in a tight loop, checking every 10 milliseconds. This starves the browser but may also be clamped by it to a larger delay (around 1 second); that's what happens in modern browsers if the tab where the page loads is inactive which is what happens if you open a link in a new tab in Chrome or Firefox. This means Preboot may start listening for events after Angular reports the app is stable and the `PrebootComplete` event fires. This then leaves the server node active, never transferring to the client one which makes for a broken site. To solve it, we're doing the following: 1. Since we want to attach event listeners as soon as possible when the server node starts being available (so that it can be shallow-cloned) we cannot wait for it to be available in full. Therefore, we switched to delegated event handlers on each of the app roots instead of directly on specific nodes. 2. As we support multiple app roots, to not duplicate large inline preboot code, we've split `getInlinePrebootCode` into two parts: function definitions to be put in `<head>` and the invocation separate for each app root put just after the app root opening tag. 3. To maintain children offset numbers (otherwise selectors won't match between the client & the server) we've removed the in-app-root script immediately when it starts executing; this won't stop the execution. `document.currentScript` is a good way to find the currently running script but it doesn't work in IE. A fallback behavior is applied to IE that leverages the fact that start scripts are invoked synchronously so the current script is the last one currently in the DOM. 4. We've removed `waitUntilReady` as there's no reliable way to wait; we'll invoke the init code immediately instead. 5. We've switched the overlay used for freeing the UI to attach to `document.documentElement` instead of `document.body` so that we don't have to wait for `body`. 6. The mentioned overlay is now created for each app root separately to avoid race conditions. Fixes angular#82 Fixes angular#72 BREAKING CHANGES: 1. When used in a non-Angular app, the code needs to be updated to use `getInlineDefinition` and `getInlineInvocation`; the previously defined `getInlinePrebootCode` has been removed. The `getInlineDefinition` output needs to be placed before any content user might interactive with, preferably in <head>. The `getInlineInvocation` output needs to be put just after the opening tag of each app root. Only `getInlineDefinition` needs to have options passed. An example expected (simplified) layout in EJS format: ```html <html> <head> <script><%= getInlineDefinition(options) %></script> </head> <body> <app1-root> <script><%= getInlineInvocation() %></script> <h2>App1 header</h2> <div>content</div> </app1-root> <app2-root> <script><%= getInlineInvocation() %></script> <h2>App2 header</h2> <span>content</span> </app2-root> </body> </html> ``` 2. The refactor breaks working with frameworks that replace elements like AngularJS with UI Router as it no longer uses serverSelector and clientSelector but expects its clientNode to remain in the DOM.

mgol · 2018-08-02T18:11:43Z

@CaerusKaru Feedback addressed. I also changed the commit message to mention the changed function names (without the PrebootCode infixes).

googlebot added the cla: yes label Apr 4, 2018

This was referenced Apr 4, 2018

Preboot suffers from race conditions #82

Closed

add condition to check that the application root is available in the DOM before prebooting #72

Closed

mgol commented Apr 4, 2018

View reviewed changes

mgol force-pushed the refactor branch from 3cafc8a to 70581c9 Compare April 4, 2018 15:29

CaerusKaru added the pr: needs review label Apr 4, 2018

CaerusKaru added this to the 6.0.0 milestone Apr 4, 2018

CaerusKaru reviewed Apr 4, 2018

View reviewed changes

CaerusKaru self-assigned this Apr 4, 2018

CaerusKaru mentioned this pull request Jul 13, 2018

build: add Bazel rules #90

Open

mgol force-pushed the refactor branch from 10ab774 to fe23836 Compare July 25, 2018 13:21

mgol changed the title ~~WIP fix: resolve Preboot race conditions~~ fix: resolve Preboot race conditions Jul 25, 2018

mgol force-pushed the refactor branch from fe23836 to c81b129 Compare July 25, 2018 13:27

CaerusKaru approved these changes Aug 2, 2018

View reviewed changes

mgol force-pushed the refactor branch from c81b129 to aeebf67 Compare August 2, 2018 18:11

CaerusKaru merged commit 1b1050b into angular:master Aug 2, 2018

mgol deleted the refactor branch August 8, 2018 19:33

mgol mentioned this pull request Aug 8, 2018

Preboot buffer: Application bootstrap on the server generated node. #91

Closed

8 tasks

fix: resolve Preboot race conditions #83

fix: resolve Preboot race conditions #83

Uh oh!

Conversation

mgol commented Apr 4, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mgol commented Apr 4, 2018

Uh oh!

mgol commented Apr 5, 2018

Uh oh!

mgol commented Jul 25, 2018

Uh oh!

CaerusKaru left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mgol commented Aug 2, 2018

Uh oh!

Uh oh!

mgol commented Apr 4, 2018 •

edited

Loading