> On July 11, 2012, 3 p.m., Robbie Gemmell wrote: > > I don't see any obvious issues with the patch, though I must admit to still > > being reluctant to change exception handling in the client this late into > > 0.18 development given it has presumably been this way since time began. As > > the behavior is ultimately controlled by a boolean, it might be an idea to > > put a system property backdoor in to allow changing the behaviour back > > later should the need arise. > > rajith attapattu wrote: > Robbie, thanks for the review. I appreciate it. > > I think the system property might be a good idea. But that would take us > back to the connection leak issue when someone decides to go the old route. > So if we add the system property, then we might have to add code to kill > the underlying connection, close all the sessions if the Session Exception is > considered 'hard' (due to the system prop) > But closing the connection was difficult as it created a deadlock :( -- > My initial attempt was to just not change the exception behavior and to close > everything.
I'm suggesting we put a backdoor in to make it behave exactly like it does now /has for probably 4 years, not to enable some other form of trying to fix the underlying problem but simply in case the sensitive'ish change late in the release process raises unforseen issues later (or anyone wants it to behave the way it does now and isn't particularly bothered by an occasional leak currently). - Robbie ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/5794/#review9058 ----------------------------------------------------------- On July 6, 2012, 12:39 a.m., rajith attapattu wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/5794/ > ----------------------------------------------------------- > > (Updated July 6, 2012, 12:39 a.m.) > > > Review request for qpid, Robbie Gemmell, Keith Wall, and Rob Godfrey. > > > Description > ------- > > When a session exception is received, we notify the connection via the > exceptionReceived method. The connection will close all sessions for this > connection, but does not close the underlying Protocol connection. > Further calls (from application code) to close the connection, does not close > the underlying protocol-connection as it skips this part, due to the > connection already being marked as closed. > This results in leaked connections. > > The proposed patch does the following. > 1. When a session exception is received, it closes the Session (and the > consumers/producers associated with it) to prevent further use of this > session. > 2. It also notifies the connection, however the exception being passed, is > marked as a soft error, thereby preventing the connection from being marked > as closed. > It also prevents the rest of the sessions being closed. > 3. The exception will however be notified via the exception listener. This is > important in certain cases, as it might be the only case where an exception > can be notified. > > What is the impact to applications. > ------------------------------------ > 1. If an application closes the connection when it receives an exception, and > then recreates the connection and it's associated objects > (sessions/consumers/producers) then this change will have no impact. > > 2. If an application just recreates everything from scratch, without > resorting to closing the connection (assuming that all exceptions received > via the listener will automatically close the connection), there will be tcp > connection leaks. > However the objects associated with the connection will eventualy be garbage > collected. We should probably close the underlying TCP connection in the > finalizer for the Connection object as a safety measure. > > With this change, the recommended approach would be, for application to check > the connection status in deciding what it should do. > For example, > > if (conn.isClosed()) > { > // re-create connection. > } > else > { > // session exception > // this means the rest of the sessions and the connection is still open. > } > > > This addresses bug QPID-3575. > https://issues.apache.org/jira/browse/QPID-3575 > > > Diffs > ----- > > > http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/client/AMQSession.java > 1357981 > > http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/client/AMQSession_0_10.java > 1357981 > > http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/common/src/main/java/org/apache/qpid/AMQException.java > 1357981 > > Diff: https://reviews.apache.org/r/5794/diff/ > > > Testing > ------- > > Tested with the default java profile and the cpp test profile. > > SelectorTest#testRuntimeSelectorError is failing as it expects the connection > to be closed. > I need to investigate further to determine if the test needs to be changed or > not. > > > Thanks, > > rajith attapattu > >
