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 30, 2024
1 parent 77f6a9b commit 0c257d7
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();
})));

return Promise.allSettled(promises).then(() => done(), done);
return Promise.allSettled(promises).then(results => done(
results.find(r => r.status === 'rejected')?.reason
));
});
}

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.find(r => r.status === 'rejected')?.reason
));
});
}

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();
})));

return Promise.allSettled(promises).then(() => done(), done);
return Promise.allSettled(promises).then(results => done(
results.find(r => r.status === 'rejected')?.reason
));
});
}

Expand Down

0 comments on commit 0c257d7

Please sign in to comment.