Fix a race condition in withAPIData()

[pento]

Description

When mapping data returned from the API to a component's props, we currently re-use data that's already stored in APIDataComponent's state. However, this causes a race condition when simultaneously receiving multiple copies of the same API request, triggered by multiple copies of the same component. The effect of this, is that some instances of the component don't receive the data sent back their API request, as demonstrated in #6277.

A simple workaround for this is to only re-use the state data when the request is complete, as this PR does.

A more comprehensive solution would probably involve merging all of the identical GET requests into one, and populating the response for all of them from a single API call. Multi-threaded caching is fraught with danger, however, and I don't think it's a blocker for this smaller fix.

[pento]

Some notes:

In the description, I quite confidently write about the root cause of this. I'm... mostly confident that's what's happening, but thanks to the wonders of dealing with race conditions, it's been a little tricky to debug and find the actual cause.

I couldn't think of a way to add a unit test for this, though we probably should.

[gziolo]

Given that we want to deprecate usage of withAPIData in favor of wp.data.select combined with resolvers, I think it's fine to skip unit tests part 👍
I saw this issue yesterday when testing other PR, but I assumed it's something with my network connection ... :)

[aduth]

I don't think this is caused by multiple instances of withAPIData. Each instance has its own state, so there shouldn't be overlap. Checking isLoading properties may address the immediate concern, though I hesitate on the consequences to reinitialize data props for a pending request.

In debugging, what ultimately seems to be the root cause is that the call to set into data prop once data is received triggers an asynchronous call to setState to merge the received values, but the call itself (before new state takes effect) triggers a cascade to componentWillReceiveProps (I'm not entirely sure why this happens; though stepping through into React source, it may have to do with React's new state handling in 16.3.0 ? ), which incurs applyMapping. At that point, this.state.dataProps does not have the merged values, and thus its own call to asynchronous setState will override the intended merged values when it is called.

I was able to have some success debugging by placing a condition breakpoint at this line with condition being dataProp.isLoading.

The snippet below seems to similarly resolve the issue. It ensures applyMapping uses state in order that it is set, asynchronously.

diff --git a/components/higher-order/with-api-data/index.js b/components/higher-order/with-api-data/index.js
index 29a24791a..475b55637 100644
--- a/components/higher-order/with-api-data/index.js
+++ b/components/higher-order/with-api-data/index.js
@@ -160,56 +160,56 @@ export default ( mapPropsToData ) => createHigherOrderComponent( ( WrappedCompon
 		}
 
 		applyMapping( props ) {
-			const { dataProps } = this.state;
-
-			const mapping = mapPropsToData( props, this.routeHelpers );
-			const nextDataProps = reduce( mapping, ( result, path, propName ) => {
-				// Skip if mapping already assigned into state data props
-				// Exmaple: Component updates with one new prop and other
-				// previously existing; previously existing should not be
-				// clobbered or re-trigger fetch
-				const dataProp = dataProps[ propName ];
-				if ( dataProp && dataProp.path === path ) {
-					result[ propName ] = dataProp;
-					return result;
-				}
+			this.setState( ( prevState ) => {
+				const mapping = mapPropsToData( props, this.routeHelpers );
+				const nextDataProps = reduce( mapping, ( result, path, propName ) => {
+					// Skip if mapping already assigned into state data props
+					// Exmaple: Component updates with one new prop and other
+					// previously existing; previously existing should not be
+					// clobbered or re-trigger fetch
+					const dataProp = result[ propName ];
+					if ( dataProp && dataProp.path === path ) {
+						result[ propName ] = dataProp;
+						return result;
+					}
 
-				result[ propName ] = {};
+					result[ propName ] = {};
 
-				const route = getRoute( this.schema, path );
-				if ( ! route ) {
-					return result;
-				}
-
-				route.methods.forEach( ( method ) => {
-					// Add request initiater into data props
-					const requestKey = this.getRequestKey( method );
-					result[ propName ][ requestKey ] = this.request.bind(
-						this,
-						propName,
-						method,
-						path
-					);
-
-					// Initialize pending flags as explicitly false
-					const pendingKey = this.getPendingKey( method );
-					result[ propName ][ pendingKey ] = false;
-
-					// If cached data already exists, populate in result
-					const cachedResponse = getCachedResponse( { path, method } );
-					if ( cachedResponse ) {
-						const dataKey = this.getResponseDataKey( method );
-						result[ propName ][ dataKey ] = cachedResponse.body;
+					const route = getRoute( this.schema, path );
+					if ( ! route ) {
+						return result;
 					}
 
-					// Track path for future map skipping
-					result[ propName ].path = path;
-				} );
+					route.methods.forEach( ( method ) => {
+						// Add request initiater into data props
+						const requestKey = this.getRequestKey( method );
+						result[ propName ][ requestKey ] = this.request.bind(
+							this,
+							propName,
+							method,
+							path
+						);
+
+						// Initialize pending flags as explicitly false
+						const pendingKey = this.getPendingKey( method );
+						result[ propName ][ pendingKey ] = false;
+
+						// If cached data already exists, populate in result
+						const cachedResponse = getCachedResponse( { path, method } );
+						if ( cachedResponse ) {
+							const dataKey = this.getResponseDataKey( method );
+							result[ propName ][ dataKey ] = cachedResponse.body;
+						}
+
+						// Track path for future map skipping
+						result[ propName ].path = path;
+					} );
 
-				return result;
-			}, {} );
+					return result;
+				}, prevState.dataProps );
 
-			this.setState( () => ( { dataProps: nextDataProps } ) );
+				return { dataProps: nextDataProps };
+			} );
 		}
 
 		render() {

[pento]

Noice, this works for me, and the UI appears snappier, too.

[pento]

Ugh, I've spent way too long trying to get the tests working here, they depend pretty heavily on the old behaviour?

@gziolo, @youknowriad, @aduth: Do you have any thoughts on fixing the tests here?

[aduth]

On brief glance, I think the failing test is to do with the change to reduce with prevState.dataProps as the initial value, which I think was meant to help with avoiding clobbering existing data (returning it to the previous {} empty object causes another test to start failing).

I'm wondering if a more direct approach might be to just disable asynchronous state updates. I'm not sure, but I don't think we're benefitting from this until AsyncComponent becomes standard anyways, and it's likely we'll have moved away from withAPIData by that point?

Diff'd with master:

diff --git a/components/higher-order/with-api-data/index.js b/components/higher-order/with-api-data/index.js
index 29a24791a..d2e5668a2 100644
--- a/components/higher-order/with-api-data/index.js
+++ b/components/higher-order/with-api-data/index.js
@@ -80,17 +80,15 @@ export default ( mapPropsToData ) => createHigherOrderComponent( ( WrappedCompon
 				return;
 			}
 
-			this.setState( ( prevState ) => {
-				const { dataProps } = prevState;
-				return {
-					dataProps: {
-						...dataProps,
-						[ propName ]: {
-							...dataProps[ propName ],
-							...values,
-						},
+			const { dataProps } = this.state;
+			this.setState( {
+				dataProps: {
+					...dataProps,
+					[ propName ]: {
+						...dataProps[ propName ],
+						...values,
 					},
-				};
+				},
 			} );
 		}
 
@@ -209,7 +207,7 @@ export default ( mapPropsToData ) => createHigherOrderComponent( ( WrappedCompon
 				return result;
 			}, {} );
 
-			this.setState( () => ( { dataProps: nextDataProps } ) );
+			this.setState( { dataProps: nextDataProps } );
 		}
 
 		render() {

[youknowriad]

Moved this to 3.0 because I'm not familiar with this code to be able to fix it myself. If it can be fixed before 2.9 is released, it's fine to include.

[adamsilverstein]

Given that we want to deprecate usage of withAPIData in favor of wp.data.select combined with resolvers

@gziolo is there an issue or PR where this effort is underway?

[youknowriad]

@adamsilverstein There's no issue at the moment but several PRs have already been merged in that vein. I think it's a good idea to have an issue to track it. This is something that has been discussed in several Data Module PRs but no dedicated issue has been created.

[youknowriad]

Related PRs #5219 #6679 #6462 #5707 #5744

[aduth]

Noting that I had looked at this again shortly before my week's vacation, and unfortunately changing to the synchronous setState was not enough to resolve observed issues. From what I recall, I was unable to come to a conclusion as to why it was insufficient.

[gziolo]

It doesn't look like it is ready for 3.1. Besides, we plan to replace all usages of withAPIData with data module. There is #7397 opened which would be a better way to use our time than fixing this temporary issue. What do you think about closing this one?

[mcsf]

@pento, feel free to reopen, but for now I'm closing based on gziolo's good suggestion!