Skip to content

Commit

Permalink
Make allSettled usage safer
Browse files Browse the repository at this point in the history
allSettled does not follow the usuage fullfil pattern: it will never
reject, and always fullfil with an array of the results of each
promises.

This is not an issue in the case of lifecycle, where we actually ignore
all errors; but it makes the code look inconsistent, as it suggests
errors are possible but not handle them.

To avoid future issues, add proper processing of the results of
allSettled to build a single error when appropriate.

Issue: BB-641
  • Loading branch information
francoisferrand committed Dec 27, 2024
1 parent 77f6a9b commit e3570fa
Show file tree
Hide file tree
Showing 2 changed files with 12 additions and 6 deletions.
6 changes: 4 additions & 2 deletions extensions/lifecycle/tasks/LifecycleTask.js
Original file line number Diff line number Diff line change
Expand Up @@ -429,7 +429,7 @@ class LifecycleTask extends BackbeatTask {

// if no versions to process, skip further processing for this batch
if (allVersionsWithStaleDate.length === 0) {
return Promise.allSettled(promises).then(() => done(), done);
return Promise.allSettled(promises).then(() => done());

Check warning on line 432 in extensions/lifecycle/tasks/LifecycleTask.js

View check run for this annotation

Codecov / codecov/patch/Backbeat

extensions/lifecycle/tasks/LifecycleTask.js#L432

Added line #L432 was not covered by tests
}

// for each version, get their relative rules, compare with
Expand Down Expand Up @@ -457,7 +457,9 @@ class LifecycleTask extends BackbeatTask {
return resolve();

Check warning on line 457 in extensions/lifecycle/tasks/LifecycleTask.js

View check run for this annotation

Codecov / codecov/patch/Backbeat

extensions/lifecycle/tasks/LifecycleTask.js#L457

Added line #L457 was not covered by tests
})));

return Promise.allSettled(promises).then(() => done(), done);
return Promise.allSettled(promises).then(results => done(
results.findLast(r => r.status === 'rejected')[0]?.reason

Check warning on line 461 in extensions/lifecycle/tasks/LifecycleTask.js

View check run for this annotation

Codecov / codecov/patch/Backbeat

extensions/lifecycle/tasks/LifecycleTask.js#L460-L461

Added lines #L460 - L461 were not covered by tests
));
});
}

Expand Down
12 changes: 8 additions & 4 deletions extensions/lifecycle/tasks/LifecycleTaskV2.js
Original file line number Diff line number Diff line change
Expand Up @@ -114,7 +114,7 @@ class LifecycleTaskV2 extends LifecycleTask {
return this.backbeatMetadataProxy.listLifecycle(listType, params, log,
(err, contents, isTruncated, markerInfo) => {
if (err) {
return Promise.allSettled(promises).then(() => done(err), () => done(err));
return Promise.allSettled(promises).then(() => done(err));

Check warning on line 117 in extensions/lifecycle/tasks/LifecycleTaskV2.js

View check run for this annotation

Codecov / codecov/patch/Backbeat

extensions/lifecycle/tasks/LifecycleTaskV2.js#L117

Added line #L117 was not covered by tests
}

// re-queue truncated listing only once.
Expand Down Expand Up @@ -145,7 +145,9 @@ class LifecycleTaskV2 extends LifecycleTask {
bucketData, bucketLCRules, contents, log,
));

return Promise.allSettled(promises).then(() => done(), done);
return Promise.allSettled(promises).then(results => done(
results.findLast(r => r.status === 'rejected')[0]?.reason

Check warning on line 149 in extensions/lifecycle/tasks/LifecycleTaskV2.js

View check run for this annotation

Codecov / codecov/patch/Backbeat

extensions/lifecycle/tasks/LifecycleTaskV2.js#L148-L149

Added lines #L148 - L149 were not covered by tests
));
});
}

Expand Down Expand Up @@ -195,7 +197,7 @@ class LifecycleTaskV2 extends LifecycleTask {
return this.backbeatMetadataProxy.listLifecycle(listType, params, log,
(err, contents, isTruncated, markerInfo) => {
if (err) {
return Promise.allSettled(promises).then(() => done(err), () => done(err));
return Promise.allSettled(promises).then(() => done(err));

Check warning on line 200 in extensions/lifecycle/tasks/LifecycleTaskV2.js

View check run for this annotation

Codecov / codecov/patch/Backbeat

extensions/lifecycle/tasks/LifecycleTaskV2.js#L200

Added line #L200 was not covered by tests
}

// create Set of unique keys not matching the next marker to
Expand Down Expand Up @@ -255,7 +257,9 @@ class LifecycleTaskV2 extends LifecycleTask {
return resolve();

Check warning on line 257 in extensions/lifecycle/tasks/LifecycleTaskV2.js

View check run for this annotation

Codecov / codecov/patch/Backbeat

extensions/lifecycle/tasks/LifecycleTaskV2.js#L257

Added line #L257 was not covered by tests
})));

return Promise.allSettled(promises).then(() => done(), done);
return Promise.allSettled(promises).then(results => done(
results.findLast(r => r.status === 'rejected')[0]?.reason

Check warning on line 261 in extensions/lifecycle/tasks/LifecycleTaskV2.js

View check run for this annotation

Codecov / codecov/patch/Backbeat

extensions/lifecycle/tasks/LifecycleTaskV2.js#L260-L261

Added lines #L260 - L261 were not covered by tests
));
});
}

Expand Down

0 comments on commit e3570fa

Please sign in to comment.