> 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. > > Robbie Gemmell wrote: > 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).
Gotcha. I will add it before I commit. - rajith ----------------------------------------------------------- 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 > >
