-
Notifications
You must be signed in to change notification settings - Fork 960
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add defensive copy for Futures allOf() method #2943
base: main
Are you sure you want to change the base?
Conversation
I've applied provided formatter again! Could you re-run the test please? Thank you in advance. |
You need to fetch the fix from #2949 |
Closes redis#2935
42ecd23
to
883d7cd
Compare
@@ -32,10 +32,11 @@ public static CompletableFuture<Void> allOf(Collection<? extends CompletionStage | |||
|
|||
LettuceAssert.notNull(stages, "Futures must not be null"); | |||
|
|||
CompletableFuture[] futures = new CompletableFuture[stages.size()]; | |||
CompletionStage[] copies = stages.toArray(new CompletionStage[0]); | |||
CompletableFuture[] futures = new CompletableFuture[copies.length]; | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why need to create a copy array if you only need to get the size?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the issue stated that this method throws ArrayIndexOutOfBoundsException. @tishun mentioned in #2935 that is probably because of the concurrent modification of the 'stages' and I agreed. So I created copy array and used in the for loop.
for (CompletionStage<?> stage : copies) { //use copies instead of stages
futures[index++] = stage.toCompletableFuture();
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, I see, thanks.
Closes #2935
I've added defensive copy for the method so that external modification could not affect stages during iteration.
As for test, I can't come up with any better idea so just used two Threads each calling allOf() and modifying stages simultaneously.
Thank you.
Make sure that:
mvn formatter:format
target. Don’t submit any formatting related changes.