Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Fix response leaks on Future.cancel #924

Merged
merged 4 commits into from
Jul 28, 2020
Merged

Fix response leaks on Future.cancel #924

merged 4 commits into from
Jul 28, 2020

Conversation

carterkozak
Copy link
Contributor

The guava methods Futures.transform, Futures.transformAsync,
Futures.catchAsync, and similar allow completion to race with
Future.cancel. This is especially dangerous when callers rely
on SettableFuture.set methods to transfer resource ownership.
Using the guava methods, it's possible successfully hand off
a result while the future ignores the value due to a cancel.

Before this PR

leak.

After this PR

==COMMIT_MSG==
Fix a race condition that can result in response leaks when dialogue requests are canceled
==COMMIT_MSG==

Possible downsides?

Concurrency is hard, this is another utility we must maintain. Compared to the cost of a resource leak this is easily worthwhile.

This introduces a new module so we can share dialogue-futures between modules. dialogue-futures is exposed only as an implementation dependency, but there's risk that it will be used directly.
There's an open question around how we expect others to interact with ListenableFuture<InputStream>.

@carterkozak
Fix response leaks on Future.cancel
4951b23 
The guava methods `Futures.transform`, `Futures.transformAsync`,
`Futures.catchAsync`, and similar allow completion to race with
`Future.cancel`. This is especially dangerous when callers rely
on `SettableFuture.set` methods to transfer resource ownership.
Using the guava methods, it's possible successfully hand off
a result while the future ignores the value due to a cancel.
@carterkozak
Add generated changelog entries
225c00f
@changelog-app
Copy link

changelog-app bot commented Jul 24, 2020 •
edited by carterkozak

Generate changelog in changelog/@unreleased

Type

  • Feature
  • Improvement
  • Fix
  • Break
  • Deprecation
  • Manual task
  • Migration

Description

Fix a race condition that can result in response leaks when dialogue requests are canceled

Check the box to generate changelog(s)

  • Generate changelog entry

@carterkozak
Copy link
Contributor Author

The race occurs any time transformed.cancel is invoked while transform.apply is running. It's unclear whether a change would be accepted upstream due to expectations around future.cancel having immediate results for other waiters.

@carterkozak
Copy link
Contributor Author

I've filed a ticket upstream, but I'm not confident there will be a change beyond perhaps documentation: google/guava#3975

iamdanfox
iamdanfox approved these changes Jul 28, 2020
View reviewed changes
Copy link
Contributor

@iamdanfox iamdanfox left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome find Carter - will be very nice to see those unbounded connection growth graphs drop down to sensible numbers :)

...-futures/src/main/java/com/palantir/dialogue/futures/DialogueDirectTransformationFuture.java
@Override
public void onFailure(Throwable throwable) {
if (input.isCancelled()) {
output.cancel(false);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Something kinda interesting here is that by calling .cancel(false) we lose the stacktrace information of who originally called the .cancel method on the original future, resulting in this very minimal stacktrace:

java.util.concurrent.CancellationException: Task was cancelled.

	at com.google.common.util.concurrent.AbstractFuture.cancellationExceptionWithCause(AbstractFuture.java:1386)
	at com.google.common.util.concurrent.AbstractFuture.getDoneValue(AbstractFuture.java:562)
	at com.google.common.util.concurrent.AbstractFuture.get(AbstractFuture.java:525)
	at com.google.common.util.concurrent.AbstractFuture$TrustedFuture.get(AbstractFuture.java:102)
	at com.palantir.dialogue.futures.DialogueDirectTransformationFuture.get(DialogueDirectTransformationFuture.java:75)
	at com.palantir.dialogue.futures.DialogueFuturesTest.testTransform_resultCancel(DialogueFuturesTest.java:54)
	at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke0(Native Method)

Compared to:



java.util.concurrent.ExecutionException: java.util.concurrent.CancellationException: Task was cancelled.

	at com.google.common.util.concurrent.AbstractFuture.getDoneValue(AbstractFuture.java:564)
	at com.google.common.util.concurrent.AbstractFuture.get(AbstractFuture.java:525)
	at com.google.common.util.concurrent.AbstractFuture$TrustedFuture.get(AbstractFuture.java:102)
	at com.palantir.dialogue.futures.DialogueDirectTransformationFuture.get(DialogueDirectTransformationFuture.java:75)
	at com.palantir.dialogue.futures.DialogueFuturesTest.testTransform_resultCancel(DialogueFuturesTest.java:54)
	at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
  ...
Caused by: java.util.concurrent.CancellationException: Task was cancelled.
	at com.google.common.util.concurrent.AbstractFuture.cancellationExceptionWithCause(AbstractFuture.java:1386)
	at com.google.common.util.concurrent.AbstractFuture.getDoneValue(AbstractFuture.java:562)
	at com.google.common.util.concurrent.AbstractFuture.get(AbstractFuture.java:525)
	at com.google.common.util.concurrent.AbstractFuture$TrustedFuture.get(AbstractFuture.java:102)
	at com.google.common.util.concurrent.Uninterruptibles.getUninterruptibly(Uninterruptibles.java:237)
	at com.google.common.util.concurrent.Futures.getDone(Futures.java:1132)
	at com.google.common.util.concurrent.Futures$CallbackListener.run(Futures.java:1081)
	at com.google.common.util.concurrent.DirectExecutor.execute(DirectExecutor.java:30)
	at com.google.common.util.concurrent.AbstractFuture.executeListener(AbstractFuture.java:1174)
	at com.google.common.util.concurrent.AbstractFuture.complete(AbstractFuture.java:969)
	at com.google.common.util.concurrent.AbstractFuture.cancel(AbstractFuture.java:623)
	at com.google.common.util.concurrent.AbstractFuture$TrustedFuture.cancel(AbstractFuture.java:130)
⭐️⭐️⭐️at com.palantir.dialogue.futures.DialogueDirectTransformationFuture.cancel(DialogueDirectTransformationFuture.java:59)
	at com.palantir.dialogue.futures.DialogueFuturesTest.testTransform_resultCancel(DialogueFuturesTest.java:52)
	... 63 more

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's correct. Cancellation in this direction is less common than the other direction.

...-futures/src/main/java/com/palantir/dialogue/futures/DialogueDirectTransformationFuture.java Outdated Show resolved Hide resolved
dialogue-futures/src/main/java/com/palantir/dialogue/futures/DialogueFutures.java Outdated Show resolved Hide resolved
...res/src/main/java/com/palantir/dialogue/futures/DialogueDirectAsyncTransformationFuture.java

@Override
public boolean cancel(boolean mayInterruptIfRunning) {
ListenableFuture<?> snapshot = currentFuture;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I assume there's some JVM reason why doing a read of the volatile field into a method-local variable is slightly more efficient??

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I structured this method to prevent future foot-guns, we wouldn't want this method to be updated to accidentally load currentFuture twice. This could just as easily use one-liner currentFuture.cancel(mayInterruptIfRunning);

dialogue-futures/src/test/java/com/palantir/dialogue/futures/DialogueFuturesTest.java
assertThat(doubled.cancel(false)).isTrue();
assertThat(doubled).isCancelled();
assertThatThrownBy(doubled::get).isInstanceOf(CancellationException.class);
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the implementation makes it pretty obvious, but I think it'd be nice to also assert original.isCancelled() just for completeness?

dialogue-futures/src/test/java/com/palantir/dialogue/futures/DialogueFuturesTest.java Outdated Show resolved Hide resolved
dialogue-futures/src/test/java/com/palantir/dialogue/futures/DialogueFuturesTest.java
assertThat(doubled.cancel(false)).isTrue();
assertThat(doubled).isCancelled();
assertThatThrownBy(doubled::get).isInstanceOf(CancellationException.class);
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

might be also nice to asset that oriignal.isCancelled, just to prove to people it definitely marks both as done?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually could this behaviour be kinda spicy if someone decided to make a future and then do two independent transformations based on it? Cancelling one of them would have the effect of cancelling the root one which ends up cancelling both?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's the desired behavior, it matches the guava method as well. When that is undesirable we use Futures.nonCancellationPropagating

dialogue-futures/src/test/java/com/palantir/dialogue/futures/DialogueFuturesTest.java Outdated Show resolved Hide resolved
dialogue-futures/src/test/java/com/palantir/dialogue/futures/DialogueFuturesTest.java
import java.util.concurrent.CancellationException;
import org.junit.jupiter.api.Test;

class DialogueFuturesTest {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I know it's a bit of a faff, but what do you think about making this test class parameterized over {DIALOGUE, GUAVA} so that we could run the same assertions against both and reassure people we're not diverging (seems like testCatchingAllAsync_originalCancel is the only one that currently fails with guava)?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added the parameterized tests. testCatchingAllAsync isn't quite a 1-1 comparison because using guava with Throwable directly will catch the CancellationException, so I've added a comment and not parameterized it.

@bulldozer-bot bulldozer-bot bot merged commit 1ab8c53 into develop Jul 28, 2020
@bulldozer-bot bulldozer-bot bot deleted the ckozak/prevent_connection_leaks branch July 28, 2020 01:27
@svc-autorelease
Copy link
Collaborator

Released 1.74.0

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@iamdanfox iamdanfox iamdanfox approved these changes

@ferozco ferozco Awaiting requested review from ferozco

Assignees
No one assigned
Labels
autorelease merge when ready
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@carterkozak @svc-autorelease @iamdanfox