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.

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

Ban all usage of Future#cancel(true) #8494

Merged
merged 1 commit into from Nov 18, 2014

Conversation

@s1monw
Copy link
Contributor

commented Nov 16, 2014

Interrupting a thread while blocking on an NIO Read / Write Operation
can cause a file to be closed due to the interrupts. This can have unpredictable
effects when files are open by index readers etc. we should prevent interruptions
across the board if possible.

@dakrone

View changes

src/main/java/org/elasticsearch/common/util/concurrent/FutureUtil.java Outdated

/**
*/
public class FutureUtil {

This comment has been minimized.

Copy link
@dakrone

dakrone Nov 17, 2014

Member

All of our utility classes are plural, and you reference FutureUtils in the forbidden apis message also, I think this should be FutureUtils

@dakrone

View changes

dev-tools/forbidden/core-signatures.txt Outdated
@@ -111,3 +111,6 @@ com.ning.compress.lzf.LZFUncompressor#<init>(com.ning.compress.DataHandler, com.
@defaultMessage Spawns a new thread which is solely under lucenes control use ThreadPool#estimatedTimeInMillisCounter instead
org.apache.lucene.search.TimeLimitingCollector#getGlobalTimerThread()
org.apache.lucene.search.TimeLimitingCollector#getGlobalCounter()

@defaultMessage Don't interrupt threads use FutureUtils#cancle(Future) instead

This comment has been minimized.

Copy link
@dakrone

dakrone Nov 17, 2014

Member

cancle -> cancel

@dakrone

This comment has been minimized.

Copy link
Member

commented Nov 17, 2014

LGTM, left two minor comments

@s1monw

This comment has been minimized.

Copy link
Contributor Author

commented Nov 17, 2014

@dakrone thanks for the review I push changes

@dakrone

This comment has been minimized.

Copy link
Member

commented Nov 17, 2014

LGTM!

@s1monw s1monw force-pushed the s1monw:ban_future_cancel branch Nov 17, 2014

@s1monw

This comment has been minimized.

Copy link
Contributor Author

commented Nov 18, 2014

@kimchy any comments on that

@kimchy

This comment has been minimized.

Copy link
Member

commented Nov 18, 2014

LGTM

[CORE] Ban all useage of Future#cancel(true)
Interrupting a thread while blocking on an NIO Read / Write Operation
can cause a file to be closed due to the interrupts. This can have unpredictable
effects when files are open by index readers etc. we should prevent interruptions
across the board if possible.

Closes #8494

@s1monw s1monw force-pushed the s1monw:ban_future_cancel branch to 5c6fe25 Nov 18, 2014

@s1monw s1monw merged commit 5c6fe25 into elastic:master Nov 18, 2014

@dakrone

This comment has been minimized.

Copy link
Member

commented Nov 19, 2014

@s1monw I think this didn't make it into 1.x? (I can't find it and FutureUtils doesn't exist on 1.x), can you backport it?

@s1monw

This comment has been minimized.

Copy link
Contributor Author

commented Nov 19, 2014

@dakrone yeah I wanted it to bake in a bit - will backport

s1monw added a commit that referenced this pull request Nov 19, 2014
[CORE] Ban all useage of Future#cancel(true)
Interrupting a thread while blocking on an NIO Read / Write Operation
can cause a file to be closed due to the interrupts. This can have unpredictable
effects when files are open by index readers etc. we should prevent interruptions
across the board if possible.

Closes #8494

Conflicts:
	src/main/java/org/elasticsearch/gateway/local/state/meta/LocalGatewayMetaState.java

@s1monw s1monw deleted the s1monw:ban_future_cancel branch Nov 19, 2014

@clintongormley clintongormley changed the title [CORE] Ban all usage of Future#cancel(true) Ban all usage of Future#cancel(true) Jun 6, 2015

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.