Skip to content

[AMQ-6763] Add timeout on transport client to avoid blocking thread #644

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

Closed
jbonofre wants to merge 1 commit into from
+2 −1
Closed

[AMQ-6763] Add timeout on transport client to avoid blocking thread #644

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion activemq-client/src/main/java/org/apache/activemq/transport/FutureResponse.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,10 +42,11 @@ public FutureResponse(ResponseCallback responseCallback, TransportFilter transpo
this.transportFilter = transportFilter;
}


public Response getResult() throws IOException {
boolean hasInterruptPending = Thread.interrupted();
try {
return responseSlot.take();
return responseSlot.poll(2, TimeUnit.MINUTES);
Copy link
Contributor

@ehossack-aws ehossack-aws Apr 2, 2021

Choose a reason for hiding this comment

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

I'm curious as to why this solves the issue?

I understand that .take() is a blocking method for the BlockingQueue type, but that's its designed behaviour right? By swapping to poll, what other operations does the thread do?

Copy link
Member Author

@jbonofre jbonofre Apr 2, 2021

Choose a reason for hiding this comment

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

Basically take() and poll() do the same thing: they call dequeue(). However, take() waits forever where poll() has a timeout. So, in the case of missing response (connection issue, whatever), the thread can be blocked forever and so the client can be stuck. The purpose of using poll() is to avoid to "block" the client thread and introduce a timeout.

Copy link
Member Author

@jbonofre jbonofre Apr 2, 2021

Choose a reason for hiding this comment

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

@ehossack-aws does it make sense to you ?

Copy link
Contributor

@ehossack-aws ehossack-aws Apr 2, 2021

Choose a reason for hiding this comment

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

It makes sense, and that matches my understanding. It just seems strange that you would call getResult() and not want/expect blocking behaviour. But I guess that's something that enables client code to do other things?

Copy link
Member Author

@jbonofre jbonofre Apr 2, 2021

Choose a reason for hiding this comment

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

Yes, getResult() is generic to any transport. It's hard to anticipate what the transport (client side) will do. However, another option is to introduce getResult(timeout timeout) to let the transport define timeout, but I bet that no transport would use it. Or, I can change getResult(timeout timeout) by default and update all transports. Let me check if I can do that easily.

Copy link
Contributor

@cshannon cshannon Apr 2, 2021

Choose a reason for hiding this comment

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

This is definitely a -1 from me the way the patch currently is without a lot of work.

It's an arbitrary timeout and and none of the code that calls this method expects a null or timeout to be returned. This needs to be configurable (and probably off by default) and anywhere that uses this would now need to expect null and properly handle it otherwise a bunch of cascading NPEs will happen. Plus tests need to be written to verify.

Copy link
Member Author

@jbonofre jbonofre Apr 2, 2021

Choose a reason for hiding this comment

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

@cshannon it's what I said previously: it needs rework, and I will postpone the fix after 5.15.15. Anyway, if you have better idea to fix the thread stuck issue, I'm open to proposal. I'm still thinking that we have to deal with a timeout on Future else we can have an accumulation on stuck thread.

Copy link
Contributor

@cshannon cshannon Apr 2, 2021

Choose a reason for hiding this comment

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

I'm not opposed to dealing with it but it's going to be a lot more complex and potential breaking changes and isn't trivial so I just wanted to point that out. There's things like failover too that would need to be tested.

Copy link
Member Author

@jbonofre jbonofre Apr 2, 2021

Choose a reason for hiding this comment

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

@cshannon +1 let me improve the PR step by step. I agree that it needs some refine and tests coverage ;)

} catch (InterruptedException e) {
hasInterruptPending = false;
throw dealWithInterrupt(e);
Expand Down