React-apollo: Infinite loop in getDataFromTree

Created on 13 May 2019  ·  6Comments  ·  Source: apollographql/react-apollo

Intended outcome:

I want to simply execute apollo in server side rendering

My code sample contains two versions : one without an apollo query (works), and one with apollo that enters in an infinite loop.

_Please notice I consider this a bug even though there is most probably an error on my side, but simply because I think if there's library misuse, the lib should reject it with a clear message, and definitely not enter in an infinite loop without complaining about anything._

app.use( (req, res) => {
    console.log("getting ", req.originalUrl, " : ", req.url)
    const client = new ApolloClient({
        ssrMode: true,
        link: createHttpLink({
            uri: 'http://localhost:4000/graphql',
            credentials: 'same-origin',
            headers: {
                cookie: req.header('Cookie'),
            },
        }),
        resolvers: {},
        cache: new InMemoryCache(),
    });
    cacheManager.client = client

    const context = {};

    const SimpleDivs = () =>
        (<div>
            in simple cases
            <div>
                rendering is ok
            </div>
        </div>
        )

    const WithQuery = () => {
        const ALL_PEOPLE = gql`
        query AllPeople {
            people {
              id
              name
            }
          }
        `
        console.log("with query....")
        return (
            <Query query={ALL_PEOPLE}>
                {({ loading, data: { people } }) =>
                    loading
                        ? <p>Loading…</p>
                        : (
                            <ul>
                                {people.map(person => <li key={person.id}>{person.name}</li>)}
                            </ul>
                        )
                }
            </Query>
        )
    }
    // The client-side App will instead use <BrowserRouter>
    const App = (
        <ApolloProvider client={client}>
            <StaticRouter location={req.url} context={context}>
                <WithQuery />
            </StaticRouter>
        </ApolloProvider>
    );

    console.log("### getDataFromTree ### to be called")
    getDataFromTree(App).then(() => {
        console.log("### getDataFromTree ### executed")
        const content = ReactDOM.renderToString(App);
        const initialState = client.extract();

        const html = <Html content={content} state={initialState} />;

        res.status(200);
        res.send(`<!doctype html>\n${ReactDOM.renderToStaticMarkup(html)}`);
        res.end();
    });
});

When I replace the component WithQuery with the SimpleDivs one, every thing works fine. but as in the code here, the app shows this log

Please notice that the code works when I remove the method above from my routes and attemp to display through the client

Actual outcome:

### getDataFromTree ### to be called
with query....
with query....
with query....
with query....
with query....
(indefinitely)

How to reproduce the issue:
I've forked the apollo template project and added server side rendering here please execute npm run startSsr instead of npm start. The server is run on http://localhost:4000

Version

bug confirmed
Source
picture ziedHamdi

Most helpful comment

Thanks @ziedHamdi and @dendrochronology for bringing this issue to our attention! I believe I have identified the problem that is causing the infinite render loop at least in the code posted here.

I reproduced the infinite loop from the issue on my machine, and I found that the source of the bug is that the ALL_PEOPLE queries created in WithQuery on each render are not === equivalent, causing react-apollo's getMarkupFromTree to process forever. I'm unsure at the moment why the graphql-tag library is outputting queries that are not === equivalent for the same input in the first place, but we are going to look into the issue of why this causes an infinite render loop.

As a workaround for now, moving the definition of ALL_PEOPLE outside of the component causes expected behavior, since the component is referencing the same query object. For example:

const ALL_PEOPLE = gql`
    query AllPeople {
        people {
            id
            name
        }
    }
`

const WithQuery = () => {
    return (
        <Query query={ALL_PEOPLE}>
            {({ loading, data: { people } }) =>
                loading
                    ? <p>Loading…</p>
                    : (
                        <ul>
                            {people.map(person => <li key={person.id}>{person.name}</li>)}
                        </ul>
                    )
            }
        </Query>
    )
}

Hopefully this workaround helps while we investigate this bug more.

picture jasonpaulos on 6 Jun 2019
👍2

All 6 comments

I haven't tried to repro @ziedHamdi's specific example yet, but I may be seeing something similar. Some of our pages use markdown-to-jsx, which works fine on the client side, but recurses infinitely down walkTree on SSR. Pages without markdown seem to render fine on the server.

I'm still trying to narrow it down further, to see if it's specific text or some such.
I opened that over at https://github.com/probablyup/markdown-to-jsx/issues/251

dendrochronology picture dendrochronology on 21 May 2019
👍1

Ignore the bit about markdown-to-jsx. I'm still seeing this, or something similar, even after removing that entirely. This is actually degrading our production environment to the point where Kubernetes kills multiple stuck pods per hour, because node has locked up the CPU.

Most SSR requests work fine. Some hang, so we tweaked our exception handling to wrap getDataFromTree in a setTimeout of 5 seconds, then fallback to a non-SSR response from the server. That's not ideal, but it at least works. Usually. Based on the request logs, I'm assuming this difference is because not all pages use the same GraphQL queries; most do a simple user/auth check, others load tons of data.

Other times the node process on the server goes to 100% CPU and stays there, and that instance can no longer serve requests. Inspecting the call stack just shows an infinite regression down walkTree.

I'd be happy to dig into this more (we really need to fix it), if some gracious soul in the Apollo community could suggest other things to look for and try. Is there a way to log/inspect the query or cache work these bad calls hit, so we can correlate? Or is this something more insidious, like a memory leak, lack of thread safety, etc. I may try to dial back some of our webpack/uglify stuff, to see if that makes inspections any more readable.

dendrochronology picture dendrochronology on 24 May 2019

My colleague @blorenz has been investigating this further, and determined that it starts with the 2.2.x branch. We'll follow up here with any details he finds.

Possibly related: we've also been trying to figure out why some query results make it into SSR output, and some don't. It's hella weird.

Example: request an article page, SSR pipeline executes the user auth check query and article content query, but the related articles query doesn't seem to return. The SSR-rendered markup shows the proper header (based on results of user auth check), and the body of the article, but the "related articles" footer at the bottom still has that React component's default placeholder text. It all works fine once things rehydrate.

dendrochronology picture dendrochronology on 6 Jun 2019

Thanks @ziedHamdi and @dendrochronology for bringing this issue to our attention! I believe I have identified the problem that is causing the infinite render loop at least in the code posted here.

I reproduced the infinite loop from the issue on my machine, and I found that the source of the bug is that the ALL_PEOPLE queries created in WithQuery on each render are not === equivalent, causing react-apollo's getMarkupFromTree to process forever. I'm unsure at the moment why the graphql-tag library is outputting queries that are not === equivalent for the same input in the first place, but we are going to look into the issue of why this causes an infinite render loop.

As a workaround for now, moving the definition of ALL_PEOPLE outside of the component causes expected behavior, since the component is referencing the same query object. For example:

const ALL_PEOPLE = gql`
    query AllPeople {
        people {
            id
            name
        }
    }
`

const WithQuery = () => {
    return (
        <Query query={ALL_PEOPLE}>
            {({ loading, data: { people } }) =>
                loading
                    ? <p>Loading…</p>
                    : (
                        <ul>
                            {people.map(person => <li key={person.id}>{person.name}</li>)}
                        </ul>
                    )
            }
        </Query>
    )
}

Hopefully this workaround helps while we investigate this bug more.

jasonpaulos picture jasonpaulos on 6 Jun 2019
👍2

After conferring with the rest of the team, we believe that this is an issue relating to the use of gql in repeated code, not an issue with the inner workings of getDataFromTree. As a best practice, you should create queries with gql a finite number of times (not in loops, render functions, or any code that can run an unbounded number of times) and treat them as constants, as this will improve performance and prevent issues like this from happening.

While getDataFromTree is partially at fault in this specific case, we've decided that we're not going to work on a fix for this issue at the moment, since there's an acceptable workaround.

If this infinite render loop continues to happen even when hoisting your gql calls to the highest level, let me know and I can investigate further.

jasonpaulos picture jasonpaulos on 6 Jun 2019
👍1

Thanks,

Things worked fine with the workaround

ziedHamdi picture ziedHamdi on 20 Jun 2019
🎉1
Was this page helpful?
0 / 5 - 0 ratings

Related issues

Acentelles picture Acentelles  ·  3Comments
tomitrescak picture tomitrescak  ·  3Comments
bdouram picture bdouram  ·  3Comments
ducarroz picture ducarroz  ·  3Comments
duckywang1 picture duckywang1  ·  3Comments