Add support for re-entrant SSR stacks by gaearon · Pull Request #13181 · facebook/react

gaearon · 2018-07-09T13:55:28Z

This is an alternative for #13158.

This embraces that we keep a stack of stacks of stacks, and explains why.

pull-bot · 2018-07-09T14:16:14Z

Details of bundled changes.

Comparing: e79366d...503ddd8

react-dom

File	Filesize Diff	Gzip Diff	Prev Size	Current Size	Prev Gzip	Current Gzip	ENV
react-dom-server.browser.development.js	+1.6%	+2.0%	99.26 KB	100.82 KB	26.19 KB	26.72 KB	UMD_DEV
react-dom-server.browser.development.js	+1.6%	+2.1%	95.39 KB	96.95 KB	25.24 KB	25.77 KB	NODE_DEV
react-dom-server.node.development.js	+1.6%	+2.0%	97.31 KB	98.87 KB	25.78 KB	26.31 KB	NODE_DEV
ReactDOMServer-dev.js	+1.7%	+2.1%	96.44 KB	98.04 KB	24.79 KB	25.31 KB	FB_WWW_DEV

Generated by 🚫 dangerJS

bvaughn

This looks okay to me. Left a minor question or two.

bvaughn · 2018-07-11T15:47:21Z

+        '    in div (at **)\n' +
+        '    in App (at **)',
+    ]);
+  });


Nice tests 👍

bvaughn · 2018-07-11T15:51:48Z

  getStackAddendum = function(): null | string {
-    if (currentDebugStack === null) {
+    const currentDebugFrame = currentDebugFrames[currentDebugFrames.length - 1];
+    if (currentDebugFrame === null) {


Just curious, but when would this happen? I don't see anywhere we push null to this stack.

Oops. Shouldn't happen, but I'll change it to test for array length instead. It's for exceptional case where we accidentally call getStackAddendum too late.

bvaughn · 2018-07-11T15:53:56Z

+function pushFrame(stack: Array<Frame>, frame: Frame): void {
+  if (__DEV__) {
+    const returnFrame =
+      stack.length > 0 ? ((stack[stack.length - 1]: any): FrameDev) : null;


Do we need this explicit DEV-only pointer? Isn't it always the frame at index - 1?

Yes, but then we need to get a reference to the frame. I could do it — it's just very confusing to think about what currentDebugStacks would mean.

I guess my solution is also confusing.

Not sure I follow. We only use the debugReturn attribute in getStackAddendum() where we just start at the top frame in currentDebugFrames and walk back. Seems like we could just do that with a for loop?

No, currentDebugFrames is not the same thing as frames :-(

That's why this is so confusing. I rewrote and pushed a more explicit solution which doesn't try to hide that we have stacks of stacks of stacks. I hope that will clarify.

Huh. The change you pushed to getStackAddendum in 5e3f51e looks like what I was suggesting 😅 but it looks like some commits are missing now so I can't step backwards....

bvaughn · 2018-07-11T15:57:35Z

+          popCurrentDebugFrame();
+        }
+      } else {
+        // Be careful! Make sure this matches the DEV path above.


Good comment 👍

gaearon · 2018-07-11T17:49:42Z

I rewrote the PR to be close to the original code, so this is just fixing the bug now. I added a bunch of comments to explain why it works this way and what it's doing exactly.

bvaughn · 2018-07-11T18:59:37Z

+    if (currentDebugStacks.length === 1) {
+      // We are entering a server renderer.
+      // Remember the previous (e.g. client) global stack implementation.
+      prevGetCurrentStackImpl = ReactDebugCurrentFrame.getCurrentStack;


Huh... does this work? Do we never push more than once before popping?

Hmm... or is this just assuming that all pushes before the pop will be for the same renderer and will have the same previous stack impl? I find this more confusing initially 😆

It's assuming that once you're inside ReactDOMServer call, you're only going to make other ReactDOMServer calls (at most), but not, e.g., ReactDOMClient or ReactART calls. Because it's synchronous. I think it's a safe assumption in 99% cases.

All ReactDOMServer calls share the same implementation. Happy to talk through it via VC — I'm worried this is getting difficult to explain like this.

Gotcha. Yeah, that's what I was thinking from reading it. Was just unsure if that would ever cause problems.

In the worst case you'll end up with no stacks. Which is exactly what's happening today with any nesting. :-)

Fair enough!

NE-SmallTown · 2018-08-13T02:26:36Z

@gaearon @bvaughn
Could anyone please give me some details about the 're-entrant' in React? I searched it about 2 years ago , 1 year ago, half a year ago through this repo, wikipedia, stackoverflow, FP programing, your youtube talks, twitter......, but I find I still can't understand it very well. Thanks!

bvaughn · 2018-08-13T15:41:24Z

Reentrancy is a general computing term:

In computing, a computer program or subroutine is called reentrant if it can be interrupted in the middle of its execution and then safely be called again before its previous invocations complete execution.

In the context of this PR, a "reentrant" call can be seen in the tests Dan added- where e.g. ReactDOMServer.renderToString() is called to render some components– and while it's rendering, one of the components being rendered also calls ReactDOMServer.renderToString() to render another set of components. The outer render stops while the inner render works, then picks back up and continues afterward.

NE-SmallTown · 2018-08-14T02:39:40Z

@bvaughn Thank you Brian!

But I had read that many times and seen questions on stackoverflow are talking about that. Almost all of them think it's about multithreading because the keyword 'interrupted' and 'safely', but in javascript it's nonexistent(except worker). The only thing I can imagine is that generator could be 'interrupted' and 'be called again', but React doesn't use generator, it use its own stack implementation(Fiber), but I don't know how to associate it with these keywords.

BTW, in javascript, we should call it 'recursion' rather than 'Reentrancy'? (Searched on google about the different in javascript but don't get answer)

bvaughn · 2018-08-14T15:47:21Z

BTW, in javascript, we should call it 'recursion' rather than 'Reentrancy'?

Recursive functions must also be reentrant, but not all reentrancy is recursive.

A function is recursive if it calls itself. For example:

function foo(shouldRecurse) {
  console.log('foo start');
  if (shouldRecurse) {
    foo(false);
  }
  console.log('foo stop');
}
foo(true);

This is not recursion, but it is an example of reentrancy:

function foo(fn) {
  console.log('foo start');
  fn();
  console.log('foo stop');
}
function bar() {
  console.log('bar');
  foo(baz);
}
function baz() {
  console.log('baz');
}
foo(bar);

NE-SmallTown · 2018-08-15T02:31:09Z

@bvaughn Thank you Brian, love it ! Before I think the second example is also a recursive function because the foo function call it self, even though it's indirect. But seems I'm wrong, we must call itself in the foo function body, can't be indirect

facebook-github-bot added the CLA Signed label Jul 9, 2018

gaearon requested a review from sebmarkbage July 9, 2018 13:55

gaearon force-pushed the ssr-more branch 2 times, most recently from 64aa1a2 to aa039da Compare July 9, 2018 14:06

gaearon force-pushed the ssr-more branch from aa039da to 0666a83 Compare July 9, 2018 14:17

bvaughn self-assigned this Jul 11, 2018

bvaughn self-requested a review July 11, 2018 14:55

gaearon mentioned this pull request Jul 11, 2018

Change warning() to automatically inject the stack, and add warningWithoutStack() as opt-out #13161

Merged

17 tasks

bvaughn approved these changes Jul 11, 2018

View reviewed changes

Add failing tests

8ecbb92

gaearon force-pushed the ssr-more branch 2 times, most recently from 5e3f51e to 0f8577d Compare July 11, 2018 17:48

gaearon force-pushed the ssr-more branch 2 times, most recently from 2eb4060 to 39568bb Compare July 11, 2018 17:57

Fix re-entrancy in ReactDOMServer

503ddd8

gaearon force-pushed the ssr-more branch from 39568bb to 503ddd8 Compare July 11, 2018 18:25

This was referenced Jul 11, 2018

Remove some statefulness from SSR stack handling #13155

Closed

Fix stack reporting with re-entrant ReactDOMServer calls #13158

Closed

gaearon merged commit 6ebc8f3 into facebook:master Jul 11, 2018

gaearon deleted the ssr-more branch July 11, 2018 18:43

bvaughn reviewed Jul 11, 2018

View reviewed changes

Conversation

gaearon commented Jul 9, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pull-bot commented Jul 9, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

react-dom

Uh oh!

bvaughn 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!

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!

gaearon commented Jul 11, 2018

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!

NE-SmallTown commented Aug 13, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bvaughn commented Aug 13, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

NE-SmallTown commented Aug 14, 2018

Uh oh!

bvaughn commented Aug 14, 2018

Uh oh!

NE-SmallTown commented Aug 15, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

gaearon commented Jul 9, 2018 •

edited

Loading

pull-bot commented Jul 9, 2018 •

edited

Loading

NE-SmallTown commented Aug 13, 2018 •

edited

Loading

bvaughn commented Aug 13, 2018 •

edited

Loading

NE-SmallTown commented Aug 15, 2018 •

edited

Loading