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

BulkRequest with listener waiting for response fails assertion 'Expected current thread [...] to not be a transport thread' #10402

Closed
nmunro-cvt opened this issue Apr 2, 2015 · 4 comments
Assignees
Labels
>bug :Core/Infra/Core Core issues without another label

Comments

@nmunro-cvt
Copy link

Starting with 1.5.0, creating a bulk request then calling bulkRequest.execute().addListener(...) results in

java.lang.AssertionError: Expected current thread [...] to not be a transport thread. Reason:

full stack trace : https://gist.github.com/nmunro-cvt/ea14d625629692ef2773

Started happening in 1.5.0, same code works fine pre-1.5.0.

Noticed some new assertions were introduced in 1.5.0

https://github.com/elastic/elasticsearch/blob/master/src/main/java/org/elasticsearch/common/util/concurrent/BaseFuture.java#L117

However, I don't see what I need to do differently to pass the assertions.

Simplified example to reproduce error: https://gist.github.com/nmunro-cvt/0be42236e9abf4359a44

@clintongormley
Copy link

@jpountz any ideas what should be changed here?

@jpountz
Copy link
Contributor

jpountz commented Apr 9, 2015

It looks to me like an actual bug that we only find about now thanks to #9164. The transport thread waits for the action to be executed so that it can run the listeners, but my understanding is that we should never wait in transport threads?

@bleskes @s1monw Since you helped reviewing on #9164 could you have a look at this issue and confirm/infirm this is a real bug?

@clintongormley clintongormley added >bug :Core/Infra/Core Core issues without another label labels Apr 12, 2015
@clintongormley clintongormley assigned bleskes and unassigned jpountz Apr 12, 2015
@clintongormley
Copy link

@bleskes assigning to you

bleskes added a commit to bleskes/elasticsearch that referenced this issue Apr 13, 2015
To protect  ourselves against running blocking operations on a network thread we have added an assertion that triggers that verifies that the thread calling a BaseFuture.get() is not a networking one. While this assert is good, it wrongly triggers when the get() is called in order to pass it's result to  a listener of AbstractListenableActionFuture who is marked to run on the same thread as the callee. At that point, we know that the operation has been completed and the get() call will not block.

To solve this, we add a tryGet() option which is guaranteed not to block.

Relates to elastic#10402
bleskes added a commit to bleskes/elasticsearch that referenced this issue Jun 3, 2015
To protect  ourselves against running blocking operations on a network thread we have added an assertion that triggers that verifies that the thread calling a BaseFuture.get() is not a networking one. While this assert is good, it wrongly triggers when the get() is called in order to pass it's result to  a listener of AbstractListenableActionFuture who is marked to run on the same thread as the callee. At that point, we know that the operation has been completed and the get() call will not block.

To solve this, we add a tryGet() option which is guaranteed not to block.

Relates to elastic#10402
bleskes added a commit that referenced this issue Jun 4, 2015
To protect  ourselves against running blocking operations on a network thread we have added an assertion that triggers that verifies that the thread calling a BaseFuture.get() is not a networking one. While this assert is good, it wrongly triggers when the get() is called in order to pass it's result to  a listener of AbstractListenableActionFuture who is marked to run on the same thread as the callee. At that point, we know that the operation has been completed and the get() call will not block.

To solve this, we change the assertion to ignore a get with a timeout of 0 and use that AbstractListenableActionFuture

Relates to #10402
Closes #10573

feedback
bleskes added a commit that referenced this issue Jun 4, 2015
To protect ourselves against running blocking operations on a network thread we have added an assertion that triggers that verifies that the thread calling a BaseFuture.get() is not a networking one. While this assert is good, it wrongly triggers when the get() is called in order to pass it's result to a listener of AbstractListenableActionFuture who is marked to run on the same thread as the callee. At that point, we know that the operation has been completed and the get() call will not block.

To solve this, we change the assertion to ignore a get with a timeout of 0 and use that AbstractListenableActionFuture

Relates to #10402
Closes #10573
bleskes added a commit that referenced this issue Jun 4, 2015
To protect ourselves against running blocking operations on a network thread we have added an assertion that triggers that verifies that the thread calling a BaseFuture.get() is not a networking one. While this assert is good, it wrongly triggers when the get() is called in order to pass it's result to a listener of AbstractListenableActionFuture who is marked to run on the same thread as the callee. At that point, we know that the operation has been completed and the get() call will not block.

To solve this, we change the assertion to ignore a get with a timeout of 0 and use that AbstractListenableActionFuture

Relates to #10402
Closes #10573
@bleskes
Copy link
Contributor

bleskes commented Jun 4, 2015

this should be fixed now with #10573 . @nmunro-cvt thx for reporting!

@bleskes bleskes closed this as completed Jun 4, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
>bug :Core/Infra/Core Core issues without another label
Projects
None yet
Development

No branches or pull requests

4 participants