AMQ-6494 Process SHUTDOWN_INFO and REMOVE_INFO to avoid timeouts

[hzbarcea]

More details regarding this bug in jira.

https://issues.apache.org/jira/browse/AMQ-6494

[tabish121]

This bit looks to filter any response that isn't a ShutdownInfo or a RemoveInfo response which I don't think you would want to do.

[hzbarcea]

@tabish121 of course you're right. I missed something in the commit, it was an incomplete optimization to avoid multiple conditions in the 'if'. Full tests now on the complete fix, coming up shortly.

That said, do you see any problem with the approach?

[tabish121]

So the question I'd have is what is really being solved here? From the context it looks like the hangs you are trying to fix relate to a client sending a command that requires a response at its root, even though the one you are targeting is RemoveInfo (ShutdownInfo never requires a response so a client would never hang on that). However RemoveInfo isn't the only command that expects a response and so a client might still hang due to the code in question if for some reason the act of the broker eventually breaking the socket on close didn't kick it out of whatever stall it is in.

It seems like a better strategy might be to just formulate a response to any incoming command that needs one if the state of the broker is stopping when the command arrives, and pass only responses when one is generated even if the broker isStopping is true. Whether or not you returned an ErrorResponse for everything when isStopping is true is one thing to think about, do you want the client to just assume things worked or do you want it to be able to report an error indicating that the broker is stopping.

As for the change you've made so far, if you were to go with that I don't know if the array based search approach is necessary vs just adding a 'isRemoveInfo' to Command to go along with 'isShutdownInfo' and just doing an plain old 'if (command.isRemoveInfo() || command.isShutdownInfo()) to that code path.

[hzbarcea]

@tabish121 good questions, I'll try to answer one by one.

1. What is being solved? Three things: one is the timeout for responses not received, second is the BrokerStoppedException which I don't think should be thrown in this case (TransportConnection.java:202), but most importantly third is that I am looking at this and other shutdown issues in an effort to bring down the unit tests time and make them more consistent.
2. Better strategy. Agree, I was looking at a minimally invasive change, but I like more what you propose. Will look into it. Comments like yours are the reason why I created this PR in the first place. Thanks.
3. The array piece. That's actually the way I had it (except I used getDataStructureType() instead of isRemoveInfo()). The two solutions are somewhat equivalent, except to add another command, in the array solution you would just add it to the array, in the other case you'd add another clause to the if. I am personally not a fan of the if/then/else overuse.

I'll take a shot at the better strategy you suggested. More elegant and correct.

[hzbarcea]

@tabish121 I created a new patch based on your suggestions. It simpler and cleaner. I'll give it another day before pushing, just in case there are other comments. Thanks!

[tabish121]

Looks good, much simpler now