handle the race condition in updating operationName #344

asiandrummer · 2017-02-23T21:07:00Z

If a new query string is passed in (and componentWillReceiveProps is called) without manually editing query (e.g. programmatically modifying the content within the editor without key events), it's possible to have both _updateQueryFacts and componentWillReceiveProps trying to update operationName by calling the setState method, most often times the one in componentWillReceiveProps winning incorrectly.

The fix is to treat the outcome from _updateQueryFacts as truth and properly updating what to update during the component lifecycle.

leebyron · 2017-02-23T23:52:25Z

src/components/GraphiQL.js

    }
+
+    return null;


This line isn't necessary, by default returning undefined will behave as expected

leebyron · 2017-02-23T23:54:15Z

src/components/GraphiQL.js

+      const newOperationName = this._updateQueryFacts(value);
+      // If `operationName` is updated as a result from updating query facts,
+      // treat it as a source of truth and update here accordingly.
+      if (newOperationName && newOperationName !== operationName) {


This is repeating the logic on lines 684 below - that seems like this could introduce more possibilities for race conditions or split logic

leebyron · 2017-02-23T23:59:00Z

src/components/GraphiQL.js

@@ -678,7 +689,11 @@ export class GraphiQL extends React.Component {
        operationName,
        ...queryFacts
      });
+
+      return operationName;


What if instead of calling setState directly, this function returned the object. That way you could replace calls to this._updateQueryFacts(query) with this.setState(this._updateQueryFacts(query)), however if you expect that multiple setState calls will occur in a row, then you can merge them before calling setState.

Another root issue seems to be that this function uses this.state as the source of truth, however clearly there are cases where operationName should not be read from this.state and instead should be passed into the function. I would suggest accepting a 2nd argument for operationName and using that instead of this.state. operationName within this function.

leebyron · 2017-02-24T00:01:41Z

src/components/GraphiQL.js

@@ -167,7 +167,12 @@ export class GraphiQL extends React.Component {
    if (nextSchema !== this.state.schema ||
        nextQuery !== this.state.query ||
        nextOperationName !== this.state.operationName) {
-      this._updateQueryFacts(nextQuery);
+      const newOperationName = this._updateQueryFacts(nextQuery);


Notice that schema can also differ from what is in this.state which will also cause a race condition. schema should also be provided as an argument to _updateQueryFacts to avoid this.

asiandrummer · 2017-02-24T02:45:13Z

@leebyron applied the changes!

leebyron · 2017-02-27T18:58:17Z

src/components/GraphiQL.js

+      if (updatedQueryAttributes !== undefined) {
+        nextOperationName = updatedQueryAttributes.operationName;
+
+        this.setState({ ...updatedQueryAttributes.queryFacts });


shouldn't this be this.setState(updatedQueryAttributes)? .queryFacts isn't a property set anywhere, the variable by that name is spread into the return value

leebyron · 2017-02-27T18:59:39Z

src/components/GraphiQL.js

    if (queryFacts) {
      // Update operation name should any query names change.
-      const operationName = getSelectedOperationName(
+      const updatedOperationName = getSelectedOperationName(
        this.state.operations,


This line should be queryFacts.operations shouldn't it... Relying on this.state.operations assumes the facts from the previous query.

asiandrummer · 2017-02-27T19:10:31Z

src/components/GraphiQL.js

      }

-      this.setState({
-        operationName,
-        ...queryFacts


shouldn't this be this.setState(updatedQueryAttributes)?

We were destructuring that object in here - we could probably be more specific with what we're setting the state with.

Yes exactly. Since it's spread in here, updatedQueryAttributes.queryFacts above won't exist, updatedQueryAttributes will contain the data spread in from the queryFacts var here

oi I see what you mean - sorry! Changed.

If a new query string is passed in (and `componentWillReceiveProps` is called) without manually editing query (e.g. programmatically modifying the content within the editor without key events), it's possible to have both `_updateQueryFacts` and `componentWillReceiveProps` tryint to update `operationName` by calling the `setState` method, most often times the one in `componentWillReceiveProps` winning incorrectly. The fix is to treat the outcome from `_updateQueryFacts` as truth and properly updating what to update during the component lifecycle. be more specific when getting object properties

leebyron · 2017-02-27T22:24:00Z

src/components/GraphiQL.js

-        this.state.operations,
-        this.state.operationName,
+      const updatedOperationName = getSelectedOperationName(
+        operations,


this should be queryFacts.operations - the operations passed in from the outside (via this.state.operations) will be derived from old information. getQueryFacts is responsible for determining that value from the query

Ah I'm wrong on this one. This is the previous operations, just was confused by the names

leebyron · 2017-02-27T22:25:16Z

src/components/GraphiQL.js

+          this.state.operations,
+          this.state.schema,
+        ),
+      );
    }
    this.setState({ query: value });


Combine this with the above to ensure one call to setState?

const queryFacts = this.updateQueryFacts(...); this.setState({ query: value, ...queryFacts })

leebyron · 2017-02-27T22:27:02Z

I'll send a follow-up PR

facebook-github-bot added the CLA Signed label Feb 23, 2017

leebyron requested changes Feb 23, 2017

View reviewed changes

leebyron reviewed Feb 24, 2017

View reviewed changes

asiandrummer force-pushed the asiandrummer-patch-2 branch 2 times, most recently from a452374 to e28f98c Compare February 24, 2017 02:44

leebyron reviewed Feb 27, 2017

View reviewed changes

asiandrummer commented Feb 27, 2017

View reviewed changes

asiandrummer force-pushed the asiandrummer-patch-2 branch 2 times, most recently from 68b5fbe to e1cce42 Compare February 27, 2017 21:47

asiandrummer force-pushed the asiandrummer-patch-2 branch from e1cce42 to 8b83e94 Compare February 27, 2017 21:49

asiandrummer merged commit 2fbde0c into master Feb 27, 2017

asiandrummer deleted the asiandrummer-patch-2 branch February 27, 2017 22:12

leebyron reviewed Feb 27, 2017

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

handle the race condition in updating operationName #344

handle the race condition in updating operationName #344

asiandrummer commented Feb 23, 2017 •

edited

Loading

leebyron Feb 23, 2017

leebyron Feb 23, 2017

leebyron Feb 23, 2017

leebyron Feb 24, 2017

asiandrummer commented Feb 24, 2017 •

edited

Loading

leebyron Feb 27, 2017

leebyron Feb 27, 2017

asiandrummer Feb 27, 2017

leebyron Feb 27, 2017

asiandrummer Feb 27, 2017

leebyron Feb 27, 2017

leebyron Feb 27, 2017

leebyron Feb 27, 2017

leebyron commented Feb 27, 2017

handle the race condition in updating operationName #344

handle the race condition in updating operationName #344

Conversation

asiandrummer commented Feb 23, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

asiandrummer commented Feb 24, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

leebyron commented Feb 27, 2017

asiandrummer commented Feb 23, 2017 •

edited

Loading

asiandrummer commented Feb 24, 2017 •

edited

Loading