Next.js: next/side-effect is not thread-safe

Created on 25 Oct 2018 · 6Comments · Source: vercel/next.js

Bug report

next/head provides much of the same functionality contained in react-helmet. Helmet is unsafe to use with SSR because of its reliance on react-side-effect, which holds state in what is essentially a global variable. next/side-effect appears to be react-side-effect with only slight modifications.

Any SSR pipeline that includes asynchronous elements is susceptible to data corruption when a Node server, like Express, is handling multiple requests asynchronously. I wrote about this here: https://open.nytimes.com/the-future-of-meta-tag-management-for-modern-react-development-ec26a7dc9183

The TL;DR is that <Helmet> (and probably <Head>) artifacts need to be scoped to each request, not tied to a global variable exposed by withSideEffect(). react-helmet-async accomplishes this via a <HelmetProvider>.

Without scoping the artifacts, pages can end up with meta tags for a different request. At The New York Times, we discovered this when reporters posted to Twitter, but the articles linked to a different story (because the meta tags were from a different request)! This is occurs when Helmet.renderStatic() or Helmet.rewind() are called while 2 requests are in flight at the same time.

To reproduce

Take an arbitrary list of URLs and save them in urls.csv. Run the following bash script in 2 separate terminal windows, which should cause requests to queue up.

#!/bin/sh
INPUT=urls.csv
while read httpRequest_requestUrl
do
  random_url=”http://localhost:3000${httpRequest_requestUrl}"
  echo “\nRequesting ${random_url}”
  curl -s ${random_url} | grep -ioE “<title[^>]+>[^<]*<\/title>”
done < $INPUT

This will mainly occur on apps that have an asynchronous data hydration step, like GraphQL-based apps that use Apollo.

I do not have a patch ready for this, but I am interested to hear thoughts around this.

p1 needs investigation

Source

staylor

👍15

Most helpful comment

This is definitely something we want to change, but it's probably going to best to do this as part of the effort to implement suspense / streaming rendering for Next.js, as next/head might have to change / be deprecated / removed in favor of pre-defining head tags on the top level, nested next/head wouldn't work when streaming rendering.