[AMQ-6763] Add timeout on transport client to avoid blocking thread#644
[AMQ-6763] Add timeout on transport client to avoid blocking thread#644jbonofre wants to merge 1 commit intoapache:mainfrom
Conversation
| boolean hasInterruptPending = Thread.interrupted(); | ||
| try { | ||
| return responseSlot.take(); | ||
| return responseSlot.poll(2, TimeUnit.MINUTES); |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
@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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
@cshannon +1 let me improve the PR step by step. I agree that it needs some refine and tests coverage ;)
|
This PR is stale because it has been open 30 days with no activity. Remove stale label or comment or this will be closed in 5 days. |
3 participants
No description provided.