Skip to content

Conversation

@jongyeol
Copy link
Contributor

@jongyeol jongyeol commented Aug 1, 2016

Make sure that:

  • You have read the contribution guidelines.

  • You use the code formatters provided here and have them applied to your changes. Don’t submit any formatting related changes.

  • You submit test cases (unit or integration tests) that back your changes.

When an AsyncCommand was cancelled in another thread, LettuceFutures#awaitOrCancel() waited for long time and a timeout exception occured. So, I updated AsyncCommand#cancel() method using latch#countDown(), it'll occured an cancelled exception directly without meaningless waiting.

Previously:

Caused by: com.lambdaworks.redis.RedisCommandTimeoutException: Command timed out
    at com.lambdaworks.redis.LettuceFutures.await(LettuceFutures.java:98)
    at com.lambdaworks.redis.LettuceFutures.awaitOrCancel(LettuceFutures.java:77)
    at com.lambdaworks.redis.commands.AsyncCommandInternalsTest.cancelWhileAwaitWithException(AsyncCommandInternalsTest.java:89)
    at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
    at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
    at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
    at java.lang.reflect.Method.invoke(Method.java:497)
    at org.junit.runners.model.FrameworkMethod$1.runReflectiveCall(FrameworkMethod.java:50)
    at org.junit.internal.runners.model.ReflectiveCallable.run(ReflectiveCallable.java:12)
    at org.junit.runners.model.FrameworkMethod.invokeExplosively(FrameworkMethod.java:47)
    at org.junit.internal.runners.statements.InvokeMethod.evaluate(InvokeMethod.java:17)
    at org.junit.internal.runners.statements.ExpectException.evaluate(ExpectException.java:19)
    ... 19 more

@coveralls
Copy link

coveralls commented Aug 1, 2016

Coverage Status

Coverage increased (+0.3%) to 93.178% when pulling a8e2219df03ed7efe3563d694426fc8ffc95a93f on jongyeol:feature/add-latch-for-cancel into 92cd88b on mp911de:master.

@mp911de
Copy link
Collaborator

mp911de commented Aug 1, 2016

Thanks for your pull request. Lettuce uses/relies currently on java.util.concurrent.CancellationException to signal canceled commands. Releasing the wait semaphore to avoid timeouts is the right approach but I'm not convinced of introducing another exception. java.util.concurrent.CancellationException is raised by the Future and would require exception translation in other places, too.

Copy link
Collaborator

Choose a reason for hiding this comment

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

The latch should be released after invoking cancellation to avoid a state in which the latch is released but the Future is not canceled yet. I think wrapping the two cancel calls into a try and releasing the latch in finally should do the job.

@jongyeol
Copy link
Contributor Author

jongyeol commented Aug 1, 2016

@mp911de , I agreed about it. then, I'll update this soon. :)

@mp911de
Copy link
Collaborator

mp911de commented Aug 1, 2016

No need to rush. I'm traveling this week, won't be able to do much.

@jongyeol jongyeol force-pushed the feature/add-latch-for-cancel branch from a8e2219 to b635f76 Compare August 2, 2016 02:14
@jongyeol jongyeol changed the title Introduce RedisCommandCancelledException for avoid waiting cancelled command Avoid timeouts for cancelling command Aug 2, 2016
@jongyeol
Copy link
Contributor Author

jongyeol commented Aug 2, 2016

I don't want to disturb your travel. Please check it after your travel~

@coveralls
Copy link

coveralls commented Aug 2, 2016

Coverage Status

Coverage increased (+0.2%) to 93.1% when pulling b635f76 on jongyeol:feature/add-latch-for-cancel into 92cd88b on mp911de:master.

mp911de pushed a commit that referenced this pull request Aug 3, 2016
Lettuce now releases the wait latch for async/future command execution if the command get canceled. Releasing the wait latch reduces the wait time and provides an eager feedback.
@mp911de
Copy link
Collaborator

mp911de commented Aug 3, 2016

Thanks for the PR. That's merged with ee3a2f7.

@mp911de mp911de closed this Aug 3, 2016
@mp911de mp911de added the type: feature A new feature label Aug 3, 2016
@mp911de mp911de added this to the Lettuce 4.3.0 milestone Aug 3, 2016
@jongyeol jongyeol deleted the feature/add-latch-for-cancel branch August 23, 2016 07:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

type: feature A new feature

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants