Skip to content

Solve race condition for test path filtering#5889

Merged
armenzg merged 1 commit intomasterfrom
fix
Jan 27, 2020
Merged

Solve race condition for test path filtering#5889
armenzg merged 1 commit intomasterfrom
fix

Conversation

@armenzg
Copy link
Copy Markdown
Collaborator

@armenzg armenzg commented Jan 27, 2020

During review time we introduced a small bug in the test filtering feature.

Reading jobList at the begining of the function instead of just before calling mapPushJobs almost guaranteed to start with an empty list which makes the call useless since it needs a non-empty list to work.

I also changed the code to call this.setState with a callback to mapPushJobs to guarantee that jobTypeNameToManifests is set in the state and not get an empty object.

diff --git a/ui/job-view/pushes/Push.jsx b/ui/job-view/pushes/Push.jsx
index 8e0e96547..0a79c394a 100644
--- a/ui/job-view/pushes/Push.jsx
+++ b/ui/job-view/pushes/Push.jsx
@@ -163,15 +163,18 @@ class Push extends React.PureComponent {

   fetchTestManifests = async () => {
     const { currentRepo, push } = this.props;
-    const { jobList } = this.state;

     const jobTypeNameToManifests = await fetchTestManifests(
       currentRepo.name,
       push.revision,
     );
-    this.setState({ jobTypeNameToManifests });
-    // This adds to the jobs the test_path property
-    this.mapPushJobs(jobList);
+    // Call setState with callback to guarantee the state of jobTypeNameToManifest
+    // to be set since it is read within mapPushJobs and we might have a race
+    // condition. We are also reading jobList now rather than before fetching
+    // the artifact because it gives us an empty list
+    this.setState({
+      jobTypeNameToManifests,
+    }, () => this.mapPushJobs(this.state.jobList));
   };

@armenzg armenzg self-assigned this Jan 27, 2020
@armenzg armenzg force-pushed the fix branch 2 times, most recently from a4e60f8 to 4417d61 Compare January 27, 2020 20:39
Solve race condition for test path filtering
e742e00 
During review time[1] we introduced a small bug in the test filtering feature.

Reading `jobList` at the begining of the function instead of just before calling `mapPushJobs` almost guaranteed to start with an empty list which makes the call useless since it needs a non-empty list to work.

I also changed the code to call `this.setState` with a callback to `mapPushJobs` to guarantee that `jobTypeNameToManifests` is set in the state and not get an empty object.

[1] #5794 (comment)
@armenzg armenzg merged commit 013c949 into master Jan 27, 2020
@armenzg armenzg deleted the fix branch January 27, 2020 21:08
alexandru-io pushed a commit to alexandru-io/treeherder that referenced this pull request Feb 17, 2020
@armenzg
Fix race condition for test path filtering (mozilla#5889)
8b6aa45 
During review time[1] we introduced a small bug in the test filtering feature.

Reading `jobList` at the begining of the function instead of just before calling `mapPushJobs` almost guaranteed to start with an empty list which makes the call useless since it needs a non-empty list to work.

I also changed the code to call `this.setState` with a callback to `mapPushJobs` to guarantee that `jobTypeNameToManifests` is set in the state and not get an empty object.

[1] mozilla#5794 (comment)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

@armenzg armenzg

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

1 participant

@armenzg