Qpid
  1. Qpid
  2. QPID-3269

Concurrently executing connections are allowed to use the same client ID

    Details

    • Type: Bug Bug
    • Status: Resolved
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: 0.6, 0.8, 0.10
    • Fix Version/s: 0.11
    • Component/s: Java Client
    • Labels:
      None

      Description

      Section 4.3.2 of the JMS spec says that the JMS provider
      should prevent concurrently executing clients from using the same client id.

      Qpid JMS client allows two connections to be created using the same client id.

      How reproducible:
      Always

      Steps to Reproduce:
      1. Create two connections with the same client ID.
      2. Observe that there are no exceptions being thrown.

      Actual results:
      No exception being thrown.

      Expected results:
      An exception should be thrown.

        Activity

        Hide
        jiraposter@reviews.apache.org added a comment -

        -----------------------------------------------------------
        This is an automatically generated e-mail. To reply, visit:
        https://reviews.apache.org/r/2880/
        -----------------------------------------------------------

        Review request for Alan Conway, Kenneth Giusti, Robbie Gemmell, and Keith Wall.

        Summary
        -------

        The c++ broker does not track the actual credit window as distinct from the remaining credit. This means that when a message-stop is received followed by a message-flow, the total number of outstanding, uncompleted messages for the subscription is larger than the window. Fixing the corner cases requires tracking some extra state in window mode. This patch modifies thing s to track the window size and the current used credit in that case. (In explicit mode we just track the available credit as before). I've moved all the credit logic out into its own pair of files and attempted to reduce a lot of the repetitiveness (once the changes needed to address the issue in the JIRA, the code was even messier than before).

        The extra state also needs to be communicated to cluster updatees, so there is a small modification to the update process and one of the update commands.

        This addresses bug QPID-3269.
        https://issues.apache.org/jira/browse/QPID-3269

        Diffs


        /trunk/qpid/cpp/src/CMakeLists.txt 1203231
        /trunk/qpid/cpp/src/Makefile.am 1203231
        /trunk/qpid/cpp/src/qpid/broker/Credit.h PRE-CREATION
        /trunk/qpid/cpp/src/qpid/broker/Credit.cpp PRE-CREATION
        /trunk/qpid/cpp/src/qpid/broker/SemanticState.h 1203231
        /trunk/qpid/cpp/src/qpid/broker/SemanticState.cpp 1203231
        /trunk/qpid/cpp/src/qpid/cluster/Cluster.cpp 1203231
        /trunk/qpid/cpp/src/qpid/cluster/Connection.h 1203231
        /trunk/qpid/cpp/src/qpid/cluster/Connection.cpp 1203231
        /trunk/qpid/cpp/src/qpid/cluster/UpdateClient.cpp 1203231
        /trunk/qpid/cpp/xml/cluster.xml 1203231
        /trunk/qpid/python/qpid/testlib.py 1203231
        /trunk/qpid/tests/src/py/qpid_tests/broker_0_10/message.py 1203231

        Diff: https://reviews.apache.org/r/2880/diff

        Testing
        -------

        New test from Keith included in the patch. All existing tests pass.

        Thanks,

        Gordon

        Show
        jiraposter@reviews.apache.org added a comment - ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/2880/ ----------------------------------------------------------- Review request for Alan Conway, Kenneth Giusti, Robbie Gemmell, and Keith Wall. Summary ------- The c++ broker does not track the actual credit window as distinct from the remaining credit. This means that when a message-stop is received followed by a message-flow, the total number of outstanding, uncompleted messages for the subscription is larger than the window. Fixing the corner cases requires tracking some extra state in window mode. This patch modifies thing s to track the window size and the current used credit in that case. (In explicit mode we just track the available credit as before). I've moved all the credit logic out into its own pair of files and attempted to reduce a lot of the repetitiveness (once the changes needed to address the issue in the JIRA, the code was even messier than before). The extra state also needs to be communicated to cluster updatees, so there is a small modification to the update process and one of the update commands. This addresses bug QPID-3269 . https://issues.apache.org/jira/browse/QPID-3269 Diffs /trunk/qpid/cpp/src/CMakeLists.txt 1203231 /trunk/qpid/cpp/src/Makefile.am 1203231 /trunk/qpid/cpp/src/qpid/broker/Credit.h PRE-CREATION /trunk/qpid/cpp/src/qpid/broker/Credit.cpp PRE-CREATION /trunk/qpid/cpp/src/qpid/broker/SemanticState.h 1203231 /trunk/qpid/cpp/src/qpid/broker/SemanticState.cpp 1203231 /trunk/qpid/cpp/src/qpid/cluster/Cluster.cpp 1203231 /trunk/qpid/cpp/src/qpid/cluster/Connection.h 1203231 /trunk/qpid/cpp/src/qpid/cluster/Connection.cpp 1203231 /trunk/qpid/cpp/src/qpid/cluster/UpdateClient.cpp 1203231 /trunk/qpid/cpp/xml/cluster.xml 1203231 /trunk/qpid/python/qpid/testlib.py 1203231 /trunk/qpid/tests/src/py/qpid_tests/broker_0_10/message.py 1203231 Diff: https://reviews.apache.org/r/2880/diff Testing ------- New test from Keith included in the patch. All existing tests pass. Thanks, Gordon
        Hide
        Rajith Attapattu added a comment -

        Fixed with a test case.

        Show
        Rajith Attapattu added a comment - Fixed with a test case.
        Hide
        jiraposter@reviews.apache.org added a comment -

        On 2011-07-07 12:14:51, Gordon Sim wrote:

        > http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/common/src/main/java/org/apache/qpid/transport/Session.java, line 1069

        > <https://reviews.apache.org/r/1027/diff/1/?file=22094#file22094line1069>

        >

        > Could you not have used sync() here instead of a new method?

        rajith attapattu wrote:

        I did wonder about this myself. But once a session is detached you can no longer sync on it right (from the broker side) ? (Bcos you need a valid session to sync on).

        Besides when the session is detached it's marked close on the client side, so sync wouldn't work anyways as we will throw a session closed exception.

        Gordon Sim wrote:

        The point would be though that if you are sync()ing on the client side and the session is detached, that already wakes you up and throws an exception, right?

        I don't understand the second sentence. However your new method is throwing a session closed exception as well.

        rajith attapattu wrote:

        Gordon, my bad, you don't need a new method sync would suffice.

        I just tested using sync with a minor modification and it works.

        Upon further testing it seems that sync() is not a good candidate here.
        The condition used by sync to go into a wait position is that maxComplete (The commands completed so far) is less than point (the command you want to sync on).
        However when we create the session both maxComplete and point are the same (i.e it's at -1) and hence it will not wait.

        We can add another condition to make sync wait if point = -1.
        Also in order to make it work, we need notify sync wait when we receive a detach/attach/close - so those methods would need to notify on the commands object.
        So can make sync work with some modifications I hinted above, but that would need more testing to ensure it doesn't have any either unintended side effects.

        sync() is used in the critical path a lot and at this juncture I think it's best to use new method rather than tinkering with sync().

        • rajith

        -----------------------------------------------------------
        This is an automatically generated e-mail. To reply, visit:
        https://reviews.apache.org/r/1027/#review985
        -----------------------------------------------------------

        On 2011-07-07 02:17:42, rajith attapattu wrote:

        -----------------------------------------------------------

        This is an automatically generated e-mail. To reply, visit:

        https://reviews.apache.org/r/1027/

        -----------------------------------------------------------

        (Updated 2011-07-07 02:17:42)

        Review request for qpid, Gordon Sim and Robbie Gemmell.

        Summary

        -------

        In order to verify the uniqueness of the client ID, a dummy session is created using client ID as it's name. This prevents any other connection from using same client ID as the session creation will fail. However this verification is switched off by default in order to preserve backwards compatibility. You need to use -Dqpid.verify_client_id=true switch verification on.

        In summary the following changes were made in order to support the above,

        1. A verifyClientID method was added to the connection delegates,

        2. AMQSession_0_10.java was modified to allow a name to be specified for the underlying AMQP session.

        3. A method was added to o.a.q.transport.Session.java to wait until the session state was changed from NEW to OPEN (or another state which triggers the error).

        4. Setter/Getter in Session.java to store/retrieve the SessionDetachCode and ConnectionDelegate to set the detach code.

        5. SessionDelegate to notify Session object when attached/dettached/closed is invoked.

        This addresses bug QPID-3269.

        https://issues.apache.org/jira/browse/QPID-3269

        Diffs

        -----

        http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/client/AMQConnection.java 1143628

        http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/client/AMQConnectionDelegate.java 1143628

        http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/client/AMQConnectionDelegate_0_10.java 1143628

        http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/client/AMQConnectionDelegate_8_0.java 1143628

        http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/client/AMQSession_0_10.java 1143628

        http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/client/XASessionImpl.java 1143628

        http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/common/src/main/java/org/apache/qpid/configuration/ClientProperties.java 1143628

        http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/common/src/main/java/org/apache/qpid/transport/ConnectionDelegate.java 1143628

        http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/common/src/main/java/org/apache/qpid/transport/Session.java 1143628

        http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/common/src/main/java/org/apache/qpid/transport/SessionDelegate.java 1143628

        Diff: https://reviews.apache.org/r/1027/diff

        Testing

        -------

        A patch containing a test will be attached to the JIRA shortly.

        Thanks,

        rajith

        Show
        jiraposter@reviews.apache.org added a comment - On 2011-07-07 12:14:51, Gordon Sim wrote: > http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/common/src/main/java/org/apache/qpid/transport/Session.java , line 1069 > < https://reviews.apache.org/r/1027/diff/1/?file=22094#file22094line1069 > > > Could you not have used sync() here instead of a new method? rajith attapattu wrote: I did wonder about this myself. But once a session is detached you can no longer sync on it right (from the broker side) ? (Bcos you need a valid session to sync on). Besides when the session is detached it's marked close on the client side, so sync wouldn't work anyways as we will throw a session closed exception. Gordon Sim wrote: The point would be though that if you are sync()ing on the client side and the session is detached, that already wakes you up and throws an exception, right? I don't understand the second sentence. However your new method is throwing a session closed exception as well. rajith attapattu wrote: Gordon, my bad, you don't need a new method sync would suffice. I just tested using sync with a minor modification and it works. Upon further testing it seems that sync() is not a good candidate here. The condition used by sync to go into a wait position is that maxComplete (The commands completed so far) is less than point (the command you want to sync on). However when we create the session both maxComplete and point are the same (i.e it's at -1) and hence it will not wait. We can add another condition to make sync wait if point = -1. Also in order to make it work, we need notify sync wait when we receive a detach/attach/close - so those methods would need to notify on the commands object. So can make sync work with some modifications I hinted above, but that would need more testing to ensure it doesn't have any either unintended side effects. sync() is used in the critical path a lot and at this juncture I think it's best to use new method rather than tinkering with sync(). rajith ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/1027/#review985 ----------------------------------------------------------- On 2011-07-07 02:17:42, rajith attapattu wrote: ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/1027/ ----------------------------------------------------------- (Updated 2011-07-07 02:17:42) Review request for qpid, Gordon Sim and Robbie Gemmell. Summary ------- In order to verify the uniqueness of the client ID, a dummy session is created using client ID as it's name. This prevents any other connection from using same client ID as the session creation will fail. However this verification is switched off by default in order to preserve backwards compatibility. You need to use -Dqpid.verify_client_id=true switch verification on. In summary the following changes were made in order to support the above, 1. A verifyClientID method was added to the connection delegates, 2. AMQSession_0_10.java was modified to allow a name to be specified for the underlying AMQP session. 3. A method was added to o.a.q.transport.Session.java to wait until the session state was changed from NEW to OPEN (or another state which triggers the error). 4. Setter/Getter in Session.java to store/retrieve the SessionDetachCode and ConnectionDelegate to set the detach code. 5. SessionDelegate to notify Session object when attached/dettached/closed is invoked. This addresses bug QPID-3269 . https://issues.apache.org/jira/browse/QPID-3269 Diffs ----- http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/client/AMQConnection.java 1143628 http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/client/AMQConnectionDelegate.java 1143628 http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/client/AMQConnectionDelegate_0_10.java 1143628 http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/client/AMQConnectionDelegate_8_0.java 1143628 http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/client/AMQSession_0_10.java 1143628 http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/client/XASessionImpl.java 1143628 http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/common/src/main/java/org/apache/qpid/configuration/ClientProperties.java 1143628 http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/common/src/main/java/org/apache/qpid/transport/ConnectionDelegate.java 1143628 http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/common/src/main/java/org/apache/qpid/transport/Session.java 1143628 http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/common/src/main/java/org/apache/qpid/transport/SessionDelegate.java 1143628 Diff: https://reviews.apache.org/r/1027/diff Testing ------- A patch containing a test will be attached to the JIRA shortly. Thanks, rajith
        Hide
        jiraposter@reviews.apache.org added a comment -

        On 2011-07-07 16:28:47, Robbie Gemmell wrote:

        > This seems a bit horrible, deliberately leaving a session open and unused. Were there really no alternative options?

        >

        > On first glance I also don't imagine this works with the Java broker, which would admittedly be the brokers fault but has it been tested? There is a MaxChannels value that gets negotiated with the broker during the AMQP connection, and the the client currently uses it to provide feedback to users when they exceed the allowed number of JMS Sessions, I think this will probably break that and allow more Sessions to be created/attempted than should be.

        >

        > When the clientid is being verified the method declares it throws JMSException from the delegate, but Exception is caught by the calling method instead. Would a boolean return not suffice here, with exceptions only being thrown from the verification method due to unexpected occurences?

        Gordon Sim wrote:

        Re "This seems a bit horrible, deliberately leaving a session open and unused. Were there really no alternative options?" - I'm to blame for that approach!

        I think if possible using a standard feature of the protocol is desirable. The only alternative I could see was some Qpid specific extension, e.g. have the client id in the properties on start-ok and have the broker understand this and reject duplicates (would need to reuse one of the limited close codes here though which isn't ideal either).

        An AMQP session is generally quite lightweight. Yes, it uses up one channel, but that's really all. The upside is that it should work for any compliant 0-10 broker.

        However, I'd genuinely be interested in alternative approaches if you have any suggestions.

        rajith attapattu wrote:

        As for the approach - I am quite keen on using something that works with any 0-10 complaint broker, rather than implementing anything qpid specific. This workaround was needed to ensure that the modification was only client side. The other alternative that Gordon mentioned requires modification to both brokers and has the disadvantage of not working with non qpid brokers.

        I also didn't quite understand about how this would cause more sessions to be created than allowed? The session created here is using the same method as any other session would. So it contains all the necessary checks and balances, including getting a channelID, registering with the connection, recreating after failover, closed when the connection is closed ..etc

        As for throwing the JMSException instead of using a boolean.

        The createSession method throws a JMSException, so either I need to handle it or pass it up the stack. I was also hoping that I don't have to handle the JMSException at all in the verifyClientID method in AMQConnection. But unfortunately we don't throw a JMSException in the AMQConnection ctor, rather an AMQException.

        So it's the same story - either I handle it at the delegate level or at the AMQConnection level. Both looks ugly.

        I think a better solution is to have the ctor in AMQConnection throw a JMSException. Our client should only throw JMSException to the application, not any Qpid specific exceptions.

        Robbie Gemmell wrote:

        Yep I was wrong about the too many Sessions, not paying attention It just means that from the users perspective they will just be able to create 1 fewer Session than if the feature is disabled, so if they know the limit is N then they will get an exception after their N-1'th Session.

        Regarding the exception, I'm not talking about whether to handle it or pass it up, just that you are catching all Exceptions when you handle it and not just the JMSException you are declaring/expecting the verify method throws. That means you are catching exceptions that may have nothing to do with the verification indicating the clientid was duplicated, but just reporting that as the problem regardless.

        This feature is disabled by default - so will not catch anybody by surprise. We can add a note in the doc about the max channel thing. The same situation arises with fialover exchange, where it creates a new session to receive updates from the failover exchange.

        I now understand your comment about the exception handling - I will change the code to specifically catch the JMSException instead of just an exception.
        If there are no further concerns I will be committing the code tonight.

        It seems we all agree that the approach taken is no worse than the suggested alternatives and probably less simpler than most.

        • rajith

        -----------------------------------------------------------
        This is an automatically generated e-mail. To reply, visit:
        https://reviews.apache.org/r/1027/#review989
        -----------------------------------------------------------

        On 2011-07-07 02:17:42, rajith attapattu wrote:

        -----------------------------------------------------------

        This is an automatically generated e-mail. To reply, visit:

        https://reviews.apache.org/r/1027/

        -----------------------------------------------------------

        (Updated 2011-07-07 02:17:42)

        Review request for qpid, Gordon Sim and Robbie Gemmell.

        Summary

        -------

        In order to verify the uniqueness of the client ID, a dummy session is created using client ID as it's name. This prevents any other connection from using same client ID as the session creation will fail. However this verification is switched off by default in order to preserve backwards compatibility. You need to use -Dqpid.verify_client_id=true switch verification on.

        In summary the following changes were made in order to support the above,

        1. A verifyClientID method was added to the connection delegates,

        2. AMQSession_0_10.java was modified to allow a name to be specified for the underlying AMQP session.

        3. A method was added to o.a.q.transport.Session.java to wait until the session state was changed from NEW to OPEN (or another state which triggers the error).

        4. Setter/Getter in Session.java to store/retrieve the SessionDetachCode and ConnectionDelegate to set the detach code.

        5. SessionDelegate to notify Session object when attached/dettached/closed is invoked.

        This addresses bug QPID-3269.

        https://issues.apache.org/jira/browse/QPID-3269

        Diffs

        -----

        http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/client/AMQConnection.java 1143628

        http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/client/AMQConnectionDelegate.java 1143628

        http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/client/AMQConnectionDelegate_0_10.java 1143628

        http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/client/AMQConnectionDelegate_8_0.java 1143628

        http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/client/AMQSession_0_10.java 1143628

        http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/client/XASessionImpl.java 1143628

        http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/common/src/main/java/org/apache/qpid/configuration/ClientProperties.java 1143628

        http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/common/src/main/java/org/apache/qpid/transport/ConnectionDelegate.java 1143628

        http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/common/src/main/java/org/apache/qpid/transport/Session.java 1143628

        http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/common/src/main/java/org/apache/qpid/transport/SessionDelegate.java 1143628

        Diff: https://reviews.apache.org/r/1027/diff

        Testing

        -------

        A patch containing a test will be attached to the JIRA shortly.

        Thanks,

        rajith

        Show
        jiraposter@reviews.apache.org added a comment - On 2011-07-07 16:28:47, Robbie Gemmell wrote: > This seems a bit horrible, deliberately leaving a session open and unused. Were there really no alternative options? > > On first glance I also don't imagine this works with the Java broker, which would admittedly be the brokers fault but has it been tested? There is a MaxChannels value that gets negotiated with the broker during the AMQP connection, and the the client currently uses it to provide feedback to users when they exceed the allowed number of JMS Sessions, I think this will probably break that and allow more Sessions to be created/attempted than should be. > > When the clientid is being verified the method declares it throws JMSException from the delegate, but Exception is caught by the calling method instead. Would a boolean return not suffice here, with exceptions only being thrown from the verification method due to unexpected occurences? Gordon Sim wrote: Re "This seems a bit horrible, deliberately leaving a session open and unused. Were there really no alternative options?" - I'm to blame for that approach! I think if possible using a standard feature of the protocol is desirable. The only alternative I could see was some Qpid specific extension, e.g. have the client id in the properties on start-ok and have the broker understand this and reject duplicates (would need to reuse one of the limited close codes here though which isn't ideal either). An AMQP session is generally quite lightweight. Yes, it uses up one channel, but that's really all. The upside is that it should work for any compliant 0-10 broker. However, I'd genuinely be interested in alternative approaches if you have any suggestions. rajith attapattu wrote: As for the approach - I am quite keen on using something that works with any 0-10 complaint broker, rather than implementing anything qpid specific. This workaround was needed to ensure that the modification was only client side. The other alternative that Gordon mentioned requires modification to both brokers and has the disadvantage of not working with non qpid brokers. I also didn't quite understand about how this would cause more sessions to be created than allowed? The session created here is using the same method as any other session would. So it contains all the necessary checks and balances, including getting a channelID, registering with the connection, recreating after failover, closed when the connection is closed ..etc As for throwing the JMSException instead of using a boolean. The createSession method throws a JMSException, so either I need to handle it or pass it up the stack. I was also hoping that I don't have to handle the JMSException at all in the verifyClientID method in AMQConnection. But unfortunately we don't throw a JMSException in the AMQConnection ctor, rather an AMQException. So it's the same story - either I handle it at the delegate level or at the AMQConnection level. Both looks ugly. I think a better solution is to have the ctor in AMQConnection throw a JMSException. Our client should only throw JMSException to the application, not any Qpid specific exceptions. Robbie Gemmell wrote: Yep I was wrong about the too many Sessions, not paying attention It just means that from the users perspective they will just be able to create 1 fewer Session than if the feature is disabled, so if they know the limit is N then they will get an exception after their N-1'th Session. Regarding the exception, I'm not talking about whether to handle it or pass it up, just that you are catching all Exceptions when you handle it and not just the JMSException you are declaring/expecting the verify method throws. That means you are catching exceptions that may have nothing to do with the verification indicating the clientid was duplicated, but just reporting that as the problem regardless. This feature is disabled by default - so will not catch anybody by surprise. We can add a note in the doc about the max channel thing. The same situation arises with fialover exchange, where it creates a new session to receive updates from the failover exchange. I now understand your comment about the exception handling - I will change the code to specifically catch the JMSException instead of just an exception. If there are no further concerns I will be committing the code tonight. It seems we all agree that the approach taken is no worse than the suggested alternatives and probably less simpler than most. rajith ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/1027/#review989 ----------------------------------------------------------- On 2011-07-07 02:17:42, rajith attapattu wrote: ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/1027/ ----------------------------------------------------------- (Updated 2011-07-07 02:17:42) Review request for qpid, Gordon Sim and Robbie Gemmell. Summary ------- In order to verify the uniqueness of the client ID, a dummy session is created using client ID as it's name. This prevents any other connection from using same client ID as the session creation will fail. However this verification is switched off by default in order to preserve backwards compatibility. You need to use -Dqpid.verify_client_id=true switch verification on. In summary the following changes were made in order to support the above, 1. A verifyClientID method was added to the connection delegates, 2. AMQSession_0_10.java was modified to allow a name to be specified for the underlying AMQP session. 3. A method was added to o.a.q.transport.Session.java to wait until the session state was changed from NEW to OPEN (or another state which triggers the error). 4. Setter/Getter in Session.java to store/retrieve the SessionDetachCode and ConnectionDelegate to set the detach code. 5. SessionDelegate to notify Session object when attached/dettached/closed is invoked. This addresses bug QPID-3269 . https://issues.apache.org/jira/browse/QPID-3269 Diffs ----- http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/client/AMQConnection.java 1143628 http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/client/AMQConnectionDelegate.java 1143628 http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/client/AMQConnectionDelegate_0_10.java 1143628 http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/client/AMQConnectionDelegate_8_0.java 1143628 http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/client/AMQSession_0_10.java 1143628 http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/client/XASessionImpl.java 1143628 http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/common/src/main/java/org/apache/qpid/configuration/ClientProperties.java 1143628 http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/common/src/main/java/org/apache/qpid/transport/ConnectionDelegate.java 1143628 http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/common/src/main/java/org/apache/qpid/transport/Session.java 1143628 http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/common/src/main/java/org/apache/qpid/transport/SessionDelegate.java 1143628 Diff: https://reviews.apache.org/r/1027/diff Testing ------- A patch containing a test will be attached to the JIRA shortly. Thanks, rajith
        Hide
        jiraposter@reviews.apache.org added a comment -

        On 2011-07-07 16:28:47, Robbie Gemmell wrote:

        > This seems a bit horrible, deliberately leaving a session open and unused. Were there really no alternative options?

        >

        > On first glance I also don't imagine this works with the Java broker, which would admittedly be the brokers fault but has it been tested? There is a MaxChannels value that gets negotiated with the broker during the AMQP connection, and the the client currently uses it to provide feedback to users when they exceed the allowed number of JMS Sessions, I think this will probably break that and allow more Sessions to be created/attempted than should be.

        >

        > When the clientid is being verified the method declares it throws JMSException from the delegate, but Exception is caught by the calling method instead. Would a boolean return not suffice here, with exceptions only being thrown from the verification method due to unexpected occurences?

        Gordon Sim wrote:

        Re "This seems a bit horrible, deliberately leaving a session open and unused. Were there really no alternative options?" - I'm to blame for that approach!

        I think if possible using a standard feature of the protocol is desirable. The only alternative I could see was some Qpid specific extension, e.g. have the client id in the properties on start-ok and have the broker understand this and reject duplicates (would need to reuse one of the limited close codes here though which isn't ideal either).

        An AMQP session is generally quite lightweight. Yes, it uses up one channel, but that's really all. The upside is that it should work for any compliant 0-10 broker.

        However, I'd genuinely be interested in alternative approaches if you have any suggestions.

        rajith attapattu wrote:

        As for the approach - I am quite keen on using something that works with any 0-10 complaint broker, rather than implementing anything qpid specific. This workaround was needed to ensure that the modification was only client side. The other alternative that Gordon mentioned requires modification to both brokers and has the disadvantage of not working with non qpid brokers.

        I also didn't quite understand about how this would cause more sessions to be created than allowed? The session created here is using the same method as any other session would. So it contains all the necessary checks and balances, including getting a channelID, registering with the connection, recreating after failover, closed when the connection is closed ..etc

        As for throwing the JMSException instead of using a boolean.

        The createSession method throws a JMSException, so either I need to handle it or pass it up the stack. I was also hoping that I don't have to handle the JMSException at all in the verifyClientID method in AMQConnection. But unfortunately we don't throw a JMSException in the AMQConnection ctor, rather an AMQException.

        So it's the same story - either I handle it at the delegate level or at the AMQConnection level. Both looks ugly.

        I think a better solution is to have the ctor in AMQConnection throw a JMSException. Our client should only throw JMSException to the application, not any Qpid specific exceptions.

        Yep I was wrong about the too many Sessions, not paying attention It just means that from the users perspective they will just be able to create 1 fewer Session than if the feature is disabled, so if they know the limit is N then they will get an exception after their N-1'th Session.

        Regarding the exception, I'm not talking about whether to handle it or pass it up, just that you are catching all Exceptions when you handle it and not just the JMSException you are declaring/expecting the verify method throws. That means you are catching exceptions that may have nothing to do with the verification indicating the clientid was duplicated, but just reporting that as the problem regardless.

        • Robbie

        -----------------------------------------------------------
        This is an automatically generated e-mail. To reply, visit:
        https://reviews.apache.org/r/1027/#review989
        -----------------------------------------------------------

        On 2011-07-07 02:17:42, rajith attapattu wrote:

        -----------------------------------------------------------

        This is an automatically generated e-mail. To reply, visit:

        https://reviews.apache.org/r/1027/

        -----------------------------------------------------------

        (Updated 2011-07-07 02:17:42)

        Review request for qpid, Gordon Sim and Robbie Gemmell.

        Summary

        -------

        In order to verify the uniqueness of the client ID, a dummy session is created using client ID as it's name. This prevents any other connection from using same client ID as the session creation will fail. However this verification is switched off by default in order to preserve backwards compatibility. You need to use -Dqpid.verify_client_id=true switch verification on.

        In summary the following changes were made in order to support the above,

        1. A verifyClientID method was added to the connection delegates,

        2. AMQSession_0_10.java was modified to allow a name to be specified for the underlying AMQP session.

        3. A method was added to o.a.q.transport.Session.java to wait until the session state was changed from NEW to OPEN (or another state which triggers the error).

        4. Setter/Getter in Session.java to store/retrieve the SessionDetachCode and ConnectionDelegate to set the detach code.

        5. SessionDelegate to notify Session object when attached/dettached/closed is invoked.

        This addresses bug QPID-3269.

        https://issues.apache.org/jira/browse/QPID-3269

        Diffs

        -----

        http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/client/AMQConnection.java 1143628

        http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/client/AMQConnectionDelegate.java 1143628

        http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/client/AMQConnectionDelegate_0_10.java 1143628

        http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/client/AMQConnectionDelegate_8_0.java 1143628

        http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/client/AMQSession_0_10.java 1143628

        http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/client/XASessionImpl.java 1143628

        http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/common/src/main/java/org/apache/qpid/configuration/ClientProperties.java 1143628

        http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/common/src/main/java/org/apache/qpid/transport/ConnectionDelegate.java 1143628

        http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/common/src/main/java/org/apache/qpid/transport/Session.java 1143628

        http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/common/src/main/java/org/apache/qpid/transport/SessionDelegate.java 1143628

        Diff: https://reviews.apache.org/r/1027/diff

        Testing

        -------

        A patch containing a test will be attached to the JIRA shortly.

        Thanks,

        rajith

        Show
        jiraposter@reviews.apache.org added a comment - On 2011-07-07 16:28:47, Robbie Gemmell wrote: > This seems a bit horrible, deliberately leaving a session open and unused. Were there really no alternative options? > > On first glance I also don't imagine this works with the Java broker, which would admittedly be the brokers fault but has it been tested? There is a MaxChannels value that gets negotiated with the broker during the AMQP connection, and the the client currently uses it to provide feedback to users when they exceed the allowed number of JMS Sessions, I think this will probably break that and allow more Sessions to be created/attempted than should be. > > When the clientid is being verified the method declares it throws JMSException from the delegate, but Exception is caught by the calling method instead. Would a boolean return not suffice here, with exceptions only being thrown from the verification method due to unexpected occurences? Gordon Sim wrote: Re "This seems a bit horrible, deliberately leaving a session open and unused. Were there really no alternative options?" - I'm to blame for that approach! I think if possible using a standard feature of the protocol is desirable. The only alternative I could see was some Qpid specific extension, e.g. have the client id in the properties on start-ok and have the broker understand this and reject duplicates (would need to reuse one of the limited close codes here though which isn't ideal either). An AMQP session is generally quite lightweight. Yes, it uses up one channel, but that's really all. The upside is that it should work for any compliant 0-10 broker. However, I'd genuinely be interested in alternative approaches if you have any suggestions. rajith attapattu wrote: As for the approach - I am quite keen on using something that works with any 0-10 complaint broker, rather than implementing anything qpid specific. This workaround was needed to ensure that the modification was only client side. The other alternative that Gordon mentioned requires modification to both brokers and has the disadvantage of not working with non qpid brokers. I also didn't quite understand about how this would cause more sessions to be created than allowed? The session created here is using the same method as any other session would. So it contains all the necessary checks and balances, including getting a channelID, registering with the connection, recreating after failover, closed when the connection is closed ..etc As for throwing the JMSException instead of using a boolean. The createSession method throws a JMSException, so either I need to handle it or pass it up the stack. I was also hoping that I don't have to handle the JMSException at all in the verifyClientID method in AMQConnection. But unfortunately we don't throw a JMSException in the AMQConnection ctor, rather an AMQException. So it's the same story - either I handle it at the delegate level or at the AMQConnection level. Both looks ugly. I think a better solution is to have the ctor in AMQConnection throw a JMSException. Our client should only throw JMSException to the application, not any Qpid specific exceptions. Yep I was wrong about the too many Sessions, not paying attention It just means that from the users perspective they will just be able to create 1 fewer Session than if the feature is disabled, so if they know the limit is N then they will get an exception after their N-1'th Session. Regarding the exception, I'm not talking about whether to handle it or pass it up, just that you are catching all Exceptions when you handle it and not just the JMSException you are declaring/expecting the verify method throws. That means you are catching exceptions that may have nothing to do with the verification indicating the clientid was duplicated, but just reporting that as the problem regardless. Robbie ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/1027/#review989 ----------------------------------------------------------- On 2011-07-07 02:17:42, rajith attapattu wrote: ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/1027/ ----------------------------------------------------------- (Updated 2011-07-07 02:17:42) Review request for qpid, Gordon Sim and Robbie Gemmell. Summary ------- In order to verify the uniqueness of the client ID, a dummy session is created using client ID as it's name. This prevents any other connection from using same client ID as the session creation will fail. However this verification is switched off by default in order to preserve backwards compatibility. You need to use -Dqpid.verify_client_id=true switch verification on. In summary the following changes were made in order to support the above, 1. A verifyClientID method was added to the connection delegates, 2. AMQSession_0_10.java was modified to allow a name to be specified for the underlying AMQP session. 3. A method was added to o.a.q.transport.Session.java to wait until the session state was changed from NEW to OPEN (or another state which triggers the error). 4. Setter/Getter in Session.java to store/retrieve the SessionDetachCode and ConnectionDelegate to set the detach code. 5. SessionDelegate to notify Session object when attached/dettached/closed is invoked. This addresses bug QPID-3269 . https://issues.apache.org/jira/browse/QPID-3269 Diffs ----- http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/client/AMQConnection.java 1143628 http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/client/AMQConnectionDelegate.java 1143628 http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/client/AMQConnectionDelegate_0_10.java 1143628 http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/client/AMQConnectionDelegate_8_0.java 1143628 http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/client/AMQSession_0_10.java 1143628 http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/client/XASessionImpl.java 1143628 http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/common/src/main/java/org/apache/qpid/configuration/ClientProperties.java 1143628 http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/common/src/main/java/org/apache/qpid/transport/ConnectionDelegate.java 1143628 http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/common/src/main/java/org/apache/qpid/transport/Session.java 1143628 http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/common/src/main/java/org/apache/qpid/transport/SessionDelegate.java 1143628 Diff: https://reviews.apache.org/r/1027/diff Testing ------- A patch containing a test will be attached to the JIRA shortly. Thanks, rajith
        Hide
        jiraposter@reviews.apache.org added a comment -

        On 2011-07-07 16:28:47, Robbie Gemmell wrote:

        > This seems a bit horrible, deliberately leaving a session open and unused. Were there really no alternative options?

        >

        > On first glance I also don't imagine this works with the Java broker, which would admittedly be the brokers fault but has it been tested? There is a MaxChannels value that gets negotiated with the broker during the AMQP connection, and the the client currently uses it to provide feedback to users when they exceed the allowed number of JMS Sessions, I think this will probably break that and allow more Sessions to be created/attempted than should be.

        >

        > When the clientid is being verified the method declares it throws JMSException from the delegate, but Exception is caught by the calling method instead. Would a boolean return not suffice here, with exceptions only being thrown from the verification method due to unexpected occurences?

        Gordon Sim wrote:

        Re "This seems a bit horrible, deliberately leaving a session open and unused. Were there really no alternative options?" - I'm to blame for that approach!

        I think if possible using a standard feature of the protocol is desirable. The only alternative I could see was some Qpid specific extension, e.g. have the client id in the properties on start-ok and have the broker understand this and reject duplicates (would need to reuse one of the limited close codes here though which isn't ideal either).

        An AMQP session is generally quite lightweight. Yes, it uses up one channel, but that's really all. The upside is that it should work for any compliant 0-10 broker.

        However, I'd genuinely be interested in alternative approaches if you have any suggestions.

        As for the approach - I am quite keen on using something that works with any 0-10 complaint broker, rather than implementing anything qpid specific. This workaround was needed to ensure that the modification was only client side. The other alternative that Gordon mentioned requires modification to both brokers and has the disadvantage of not working with non qpid brokers.

        I also didn't quite understand about how this would cause more sessions to be created than allowed? The session created here is using the same method as any other session would. So it contains all the necessary checks and balances, including getting a channelID, registering with the connection, recreating after failover, closed when the connection is closed ..etc

        As for throwing the JMSException instead of using a boolean.
        The createSession method throws a JMSException, so either I need to handle it or pass it up the stack. I was also hoping that I don't have to handle the JMSException at all in the verifyClientID method in AMQConnection. But unfortunately we don't throw a JMSException in the AMQConnection ctor, rather an AMQException.
        So it's the same story - either I handle it at the delegate level or at the AMQConnection level. Both looks ugly.

        I think a better solution is to have the ctor in AMQConnection throw a JMSException. Our client should only throw JMSException to the application, not any Qpid specific exceptions.

        • rajith

        -----------------------------------------------------------
        This is an automatically generated e-mail. To reply, visit:
        https://reviews.apache.org/r/1027/#review989
        -----------------------------------------------------------

        On 2011-07-07 02:17:42, rajith attapattu wrote:

        -----------------------------------------------------------

        This is an automatically generated e-mail. To reply, visit:

        https://reviews.apache.org/r/1027/

        -----------------------------------------------------------

        (Updated 2011-07-07 02:17:42)

        Review request for qpid, Gordon Sim and Robbie Gemmell.

        Summary

        -------

        In order to verify the uniqueness of the client ID, a dummy session is created using client ID as it's name. This prevents any other connection from using same client ID as the session creation will fail. However this verification is switched off by default in order to preserve backwards compatibility. You need to use -Dqpid.verify_client_id=true switch verification on.

        In summary the following changes were made in order to support the above,

        1. A verifyClientID method was added to the connection delegates,

        2. AMQSession_0_10.java was modified to allow a name to be specified for the underlying AMQP session.

        3. A method was added to o.a.q.transport.Session.java to wait until the session state was changed from NEW to OPEN (or another state which triggers the error).

        4. Setter/Getter in Session.java to store/retrieve the SessionDetachCode and ConnectionDelegate to set the detach code.

        5. SessionDelegate to notify Session object when attached/dettached/closed is invoked.

        This addresses bug QPID-3269.

        https://issues.apache.org/jira/browse/QPID-3269

        Diffs

        -----

        http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/client/AMQConnection.java 1143628

        http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/client/AMQConnectionDelegate.java 1143628

        http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/client/AMQConnectionDelegate_0_10.java 1143628

        http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/client/AMQConnectionDelegate_8_0.java 1143628

        http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/client/AMQSession_0_10.java 1143628

        http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/client/XASessionImpl.java 1143628

        http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/common/src/main/java/org/apache/qpid/configuration/ClientProperties.java 1143628

        http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/common/src/main/java/org/apache/qpid/transport/ConnectionDelegate.java 1143628

        http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/common/src/main/java/org/apache/qpid/transport/Session.java 1143628

        http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/common/src/main/java/org/apache/qpid/transport/SessionDelegate.java 1143628

        Diff: https://reviews.apache.org/r/1027/diff

        Testing

        -------

        A patch containing a test will be attached to the JIRA shortly.

        Thanks,

        rajith

        Show
        jiraposter@reviews.apache.org added a comment - On 2011-07-07 16:28:47, Robbie Gemmell wrote: > This seems a bit horrible, deliberately leaving a session open and unused. Were there really no alternative options? > > On first glance I also don't imagine this works with the Java broker, which would admittedly be the brokers fault but has it been tested? There is a MaxChannels value that gets negotiated with the broker during the AMQP connection, and the the client currently uses it to provide feedback to users when they exceed the allowed number of JMS Sessions, I think this will probably break that and allow more Sessions to be created/attempted than should be. > > When the clientid is being verified the method declares it throws JMSException from the delegate, but Exception is caught by the calling method instead. Would a boolean return not suffice here, with exceptions only being thrown from the verification method due to unexpected occurences? Gordon Sim wrote: Re "This seems a bit horrible, deliberately leaving a session open and unused. Were there really no alternative options?" - I'm to blame for that approach! I think if possible using a standard feature of the protocol is desirable. The only alternative I could see was some Qpid specific extension, e.g. have the client id in the properties on start-ok and have the broker understand this and reject duplicates (would need to reuse one of the limited close codes here though which isn't ideal either). An AMQP session is generally quite lightweight. Yes, it uses up one channel, but that's really all. The upside is that it should work for any compliant 0-10 broker. However, I'd genuinely be interested in alternative approaches if you have any suggestions. As for the approach - I am quite keen on using something that works with any 0-10 complaint broker, rather than implementing anything qpid specific. This workaround was needed to ensure that the modification was only client side. The other alternative that Gordon mentioned requires modification to both brokers and has the disadvantage of not working with non qpid brokers. I also didn't quite understand about how this would cause more sessions to be created than allowed? The session created here is using the same method as any other session would. So it contains all the necessary checks and balances, including getting a channelID, registering with the connection, recreating after failover, closed when the connection is closed ..etc As for throwing the JMSException instead of using a boolean. The createSession method throws a JMSException, so either I need to handle it or pass it up the stack. I was also hoping that I don't have to handle the JMSException at all in the verifyClientID method in AMQConnection. But unfortunately we don't throw a JMSException in the AMQConnection ctor, rather an AMQException. So it's the same story - either I handle it at the delegate level or at the AMQConnection level. Both looks ugly. I think a better solution is to have the ctor in AMQConnection throw a JMSException. Our client should only throw JMSException to the application, not any Qpid specific exceptions. rajith ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/1027/#review989 ----------------------------------------------------------- On 2011-07-07 02:17:42, rajith attapattu wrote: ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/1027/ ----------------------------------------------------------- (Updated 2011-07-07 02:17:42) Review request for qpid, Gordon Sim and Robbie Gemmell. Summary ------- In order to verify the uniqueness of the client ID, a dummy session is created using client ID as it's name. This prevents any other connection from using same client ID as the session creation will fail. However this verification is switched off by default in order to preserve backwards compatibility. You need to use -Dqpid.verify_client_id=true switch verification on. In summary the following changes were made in order to support the above, 1. A verifyClientID method was added to the connection delegates, 2. AMQSession_0_10.java was modified to allow a name to be specified for the underlying AMQP session. 3. A method was added to o.a.q.transport.Session.java to wait until the session state was changed from NEW to OPEN (or another state which triggers the error). 4. Setter/Getter in Session.java to store/retrieve the SessionDetachCode and ConnectionDelegate to set the detach code. 5. SessionDelegate to notify Session object when attached/dettached/closed is invoked. This addresses bug QPID-3269 . https://issues.apache.org/jira/browse/QPID-3269 Diffs ----- http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/client/AMQConnection.java 1143628 http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/client/AMQConnectionDelegate.java 1143628 http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/client/AMQConnectionDelegate_0_10.java 1143628 http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/client/AMQConnectionDelegate_8_0.java 1143628 http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/client/AMQSession_0_10.java 1143628 http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/client/XASessionImpl.java 1143628 http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/common/src/main/java/org/apache/qpid/configuration/ClientProperties.java 1143628 http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/common/src/main/java/org/apache/qpid/transport/ConnectionDelegate.java 1143628 http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/common/src/main/java/org/apache/qpid/transport/Session.java 1143628 http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/common/src/main/java/org/apache/qpid/transport/SessionDelegate.java 1143628 Diff: https://reviews.apache.org/r/1027/diff Testing ------- A patch containing a test will be attached to the JIRA shortly. Thanks, rajith
        Hide
        jiraposter@reviews.apache.org added a comment -

        On 2011-07-07 16:28:47, Robbie Gemmell wrote:

        > This seems a bit horrible, deliberately leaving a session open and unused. Were there really no alternative options?

        >

        > On first glance I also don't imagine this works with the Java broker, which would admittedly be the brokers fault but has it been tested? There is a MaxChannels value that gets negotiated with the broker during the AMQP connection, and the the client currently uses it to provide feedback to users when they exceed the allowed number of JMS Sessions, I think this will probably break that and allow more Sessions to be created/attempted than should be.

        >

        > When the clientid is being verified the method declares it throws JMSException from the delegate, but Exception is caught by the calling method instead. Would a boolean return not suffice here, with exceptions only being thrown from the verification method due to unexpected occurences?

        Re "This seems a bit horrible, deliberately leaving a session open and unused. Were there really no alternative options?" - I'm to blame for that approach!

        I think if possible using a standard feature of the protocol is desirable. The only alternative I could see was some Qpid specific extension, e.g. have the client id in the properties on start-ok and have the broker understand this and reject duplicates (would need to reuse one of the limited close codes here though which isn't ideal either).

        An AMQP session is generally quite lightweight. Yes, it uses up one channel, but that's really all. The upside is that it should work for any compliant 0-10 broker.

        However, I'd genuinely be interested in alternative approaches if you have any suggestions.

        • Gordon

        -----------------------------------------------------------
        This is an automatically generated e-mail. To reply, visit:
        https://reviews.apache.org/r/1027/#review989
        -----------------------------------------------------------

        On 2011-07-07 02:17:42, rajith attapattu wrote:

        -----------------------------------------------------------

        This is an automatically generated e-mail. To reply, visit:

        https://reviews.apache.org/r/1027/

        -----------------------------------------------------------

        (Updated 2011-07-07 02:17:42)

        Review request for qpid, Gordon Sim and Robbie Gemmell.

        Summary

        -------

        In order to verify the uniqueness of the client ID, a dummy session is created using client ID as it's name. This prevents any other connection from using same client ID as the session creation will fail. However this verification is switched off by default in order to preserve backwards compatibility. You need to use -Dqpid.verify_client_id=true switch verification on.

        In summary the following changes were made in order to support the above,

        1. A verifyClientID method was added to the connection delegates,

        2. AMQSession_0_10.java was modified to allow a name to be specified for the underlying AMQP session.

        3. A method was added to o.a.q.transport.Session.java to wait until the session state was changed from NEW to OPEN (or another state which triggers the error).

        4. Setter/Getter in Session.java to store/retrieve the SessionDetachCode and ConnectionDelegate to set the detach code.

        5. SessionDelegate to notify Session object when attached/dettached/closed is invoked.

        This addresses bug QPID-3269.

        https://issues.apache.org/jira/browse/QPID-3269

        Diffs

        -----

        http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/client/AMQConnection.java 1143628

        http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/client/AMQConnectionDelegate.java 1143628

        http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/client/AMQConnectionDelegate_0_10.java 1143628

        http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/client/AMQConnectionDelegate_8_0.java 1143628

        http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/client/AMQSession_0_10.java 1143628

        http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/client/XASessionImpl.java 1143628

        http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/common/src/main/java/org/apache/qpid/configuration/ClientProperties.java 1143628

        http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/common/src/main/java/org/apache/qpid/transport/ConnectionDelegate.java 1143628

        http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/common/src/main/java/org/apache/qpid/transport/Session.java 1143628

        http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/common/src/main/java/org/apache/qpid/transport/SessionDelegate.java 1143628

        Diff: https://reviews.apache.org/r/1027/diff

        Testing

        -------

        A patch containing a test will be attached to the JIRA shortly.

        Thanks,

        rajith

        Show
        jiraposter@reviews.apache.org added a comment - On 2011-07-07 16:28:47, Robbie Gemmell wrote: > This seems a bit horrible, deliberately leaving a session open and unused. Were there really no alternative options? > > On first glance I also don't imagine this works with the Java broker, which would admittedly be the brokers fault but has it been tested? There is a MaxChannels value that gets negotiated with the broker during the AMQP connection, and the the client currently uses it to provide feedback to users when they exceed the allowed number of JMS Sessions, I think this will probably break that and allow more Sessions to be created/attempted than should be. > > When the clientid is being verified the method declares it throws JMSException from the delegate, but Exception is caught by the calling method instead. Would a boolean return not suffice here, with exceptions only being thrown from the verification method due to unexpected occurences? Re "This seems a bit horrible, deliberately leaving a session open and unused. Were there really no alternative options?" - I'm to blame for that approach! I think if possible using a standard feature of the protocol is desirable. The only alternative I could see was some Qpid specific extension, e.g. have the client id in the properties on start-ok and have the broker understand this and reject duplicates (would need to reuse one of the limited close codes here though which isn't ideal either). An AMQP session is generally quite lightweight. Yes, it uses up one channel, but that's really all. The upside is that it should work for any compliant 0-10 broker. However, I'd genuinely be interested in alternative approaches if you have any suggestions. Gordon ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/1027/#review989 ----------------------------------------------------------- On 2011-07-07 02:17:42, rajith attapattu wrote: ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/1027/ ----------------------------------------------------------- (Updated 2011-07-07 02:17:42) Review request for qpid, Gordon Sim and Robbie Gemmell. Summary ------- In order to verify the uniqueness of the client ID, a dummy session is created using client ID as it's name. This prevents any other connection from using same client ID as the session creation will fail. However this verification is switched off by default in order to preserve backwards compatibility. You need to use -Dqpid.verify_client_id=true switch verification on. In summary the following changes were made in order to support the above, 1. A verifyClientID method was added to the connection delegates, 2. AMQSession_0_10.java was modified to allow a name to be specified for the underlying AMQP session. 3. A method was added to o.a.q.transport.Session.java to wait until the session state was changed from NEW to OPEN (or another state which triggers the error). 4. Setter/Getter in Session.java to store/retrieve the SessionDetachCode and ConnectionDelegate to set the detach code. 5. SessionDelegate to notify Session object when attached/dettached/closed is invoked. This addresses bug QPID-3269 . https://issues.apache.org/jira/browse/QPID-3269 Diffs ----- http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/client/AMQConnection.java 1143628 http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/client/AMQConnectionDelegate.java 1143628 http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/client/AMQConnectionDelegate_0_10.java 1143628 http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/client/AMQConnectionDelegate_8_0.java 1143628 http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/client/AMQSession_0_10.java 1143628 http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/client/XASessionImpl.java 1143628 http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/common/src/main/java/org/apache/qpid/configuration/ClientProperties.java 1143628 http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/common/src/main/java/org/apache/qpid/transport/ConnectionDelegate.java 1143628 http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/common/src/main/java/org/apache/qpid/transport/Session.java 1143628 http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/common/src/main/java/org/apache/qpid/transport/SessionDelegate.java 1143628 Diff: https://reviews.apache.org/r/1027/diff Testing ------- A patch containing a test will be attached to the JIRA shortly. Thanks, rajith
        Hide
        jiraposter@reviews.apache.org added a comment -

        -----------------------------------------------------------
        This is an automatically generated e-mail. To reply, visit:
        https://reviews.apache.org/r/1027/#review989
        -----------------------------------------------------------

        This seems a bit horrible, deliberately leaving a session open and unused. Were there really no alternative options?

        On first glance I also don't imagine this works with the Java broker, which would admittedly be the brokers fault but has it been tested? There is a MaxChannels value that gets negotiated with the broker during the AMQP connection, and the the client currently uses it to provide feedback to users when they exceed the allowed number of JMS Sessions, I think this will probably break that and allow more Sessions to be created/attempted than should be.

        When the clientid is being verified the method declares it throws JMSException from the delegate, but Exception is caught by the calling method instead. Would a boolean return not suffice here, with exceptions only being thrown from the verification method due to unexpected occurences?

        • Robbie

        On 2011-07-07 02:17:42, rajith attapattu wrote:

        -----------------------------------------------------------

        This is an automatically generated e-mail. To reply, visit:

        https://reviews.apache.org/r/1027/

        -----------------------------------------------------------

        (Updated 2011-07-07 02:17:42)

        Review request for qpid, Gordon Sim and Robbie Gemmell.

        Summary

        -------

        In order to verify the uniqueness of the client ID, a dummy session is created using client ID as it's name. This prevents any other connection from using same client ID as the session creation will fail. However this verification is switched off by default in order to preserve backwards compatibility. You need to use -Dqpid.verify_client_id=true switch verification on.

        In summary the following changes were made in order to support the above,

        1. A verifyClientID method was added to the connection delegates,

        2. AMQSession_0_10.java was modified to allow a name to be specified for the underlying AMQP session.

        3. A method was added to o.a.q.transport.Session.java to wait until the session state was changed from NEW to OPEN (or another state which triggers the error).

        4. Setter/Getter in Session.java to store/retrieve the SessionDetachCode and ConnectionDelegate to set the detach code.

        5. SessionDelegate to notify Session object when attached/dettached/closed is invoked.

        This addresses bug QPID-3269.

        https://issues.apache.org/jira/browse/QPID-3269

        Diffs

        -----

        http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/client/AMQConnection.java 1143628

        http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/client/AMQConnectionDelegate.java 1143628

        http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/client/AMQConnectionDelegate_0_10.java 1143628

        http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/client/AMQConnectionDelegate_8_0.java 1143628

        http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/client/AMQSession_0_10.java 1143628

        http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/client/XASessionImpl.java 1143628

        http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/common/src/main/java/org/apache/qpid/configuration/ClientProperties.java 1143628

        http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/common/src/main/java/org/apache/qpid/transport/ConnectionDelegate.java 1143628

        http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/common/src/main/java/org/apache/qpid/transport/Session.java 1143628

        http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/common/src/main/java/org/apache/qpid/transport/SessionDelegate.java 1143628

        Diff: https://reviews.apache.org/r/1027/diff

        Testing

        -------

        A patch containing a test will be attached to the JIRA shortly.

        Thanks,

        rajith

        Show
        jiraposter@reviews.apache.org added a comment - ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/1027/#review989 ----------------------------------------------------------- This seems a bit horrible, deliberately leaving a session open and unused. Were there really no alternative options? On first glance I also don't imagine this works with the Java broker, which would admittedly be the brokers fault but has it been tested? There is a MaxChannels value that gets negotiated with the broker during the AMQP connection, and the the client currently uses it to provide feedback to users when they exceed the allowed number of JMS Sessions, I think this will probably break that and allow more Sessions to be created/attempted than should be. When the clientid is being verified the method declares it throws JMSException from the delegate, but Exception is caught by the calling method instead. Would a boolean return not suffice here, with exceptions only being thrown from the verification method due to unexpected occurences? Robbie On 2011-07-07 02:17:42, rajith attapattu wrote: ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/1027/ ----------------------------------------------------------- (Updated 2011-07-07 02:17:42) Review request for qpid, Gordon Sim and Robbie Gemmell. Summary ------- In order to verify the uniqueness of the client ID, a dummy session is created using client ID as it's name. This prevents any other connection from using same client ID as the session creation will fail. However this verification is switched off by default in order to preserve backwards compatibility. You need to use -Dqpid.verify_client_id=true switch verification on. In summary the following changes were made in order to support the above, 1. A verifyClientID method was added to the connection delegates, 2. AMQSession_0_10.java was modified to allow a name to be specified for the underlying AMQP session. 3. A method was added to o.a.q.transport.Session.java to wait until the session state was changed from NEW to OPEN (or another state which triggers the error). 4. Setter/Getter in Session.java to store/retrieve the SessionDetachCode and ConnectionDelegate to set the detach code. 5. SessionDelegate to notify Session object when attached/dettached/closed is invoked. This addresses bug QPID-3269 . https://issues.apache.org/jira/browse/QPID-3269 Diffs ----- http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/client/AMQConnection.java 1143628 http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/client/AMQConnectionDelegate.java 1143628 http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/client/AMQConnectionDelegate_0_10.java 1143628 http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/client/AMQConnectionDelegate_8_0.java 1143628 http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/client/AMQSession_0_10.java 1143628 http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/client/XASessionImpl.java 1143628 http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/common/src/main/java/org/apache/qpid/configuration/ClientProperties.java 1143628 http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/common/src/main/java/org/apache/qpid/transport/ConnectionDelegate.java 1143628 http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/common/src/main/java/org/apache/qpid/transport/Session.java 1143628 http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/common/src/main/java/org/apache/qpid/transport/SessionDelegate.java 1143628 Diff: https://reviews.apache.org/r/1027/diff Testing ------- A patch containing a test will be attached to the JIRA shortly. Thanks, rajith
        Hide
        jiraposter@reviews.apache.org added a comment -

        On 2011-07-07 12:14:51, Gordon Sim wrote:

        > http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/common/src/main/java/org/apache/qpid/transport/Session.java, line 1069

        > <https://reviews.apache.org/r/1027/diff/1/?file=22094#file22094line1069>

        >

        > Could you not have used sync() here instead of a new method?

        rajith attapattu wrote:

        I did wonder about this myself. But once a session is detached you can no longer sync on it right (from the broker side) ? (Bcos you need a valid session to sync on).

        Besides when the session is detached it's marked close on the client side, so sync wouldn't work anyways as we will throw a session closed exception.

        Gordon Sim wrote:

        The point would be though that if you are sync()ing on the client side and the session is detached, that already wakes you up and throws an exception, right?

        I don't understand the second sentence. However your new method is throwing a session closed exception as well.

        Gordon, my bad, you don't need a new method sync would suffice.
        I just tested using sync with a minor modification and it works.

        • rajith

        -----------------------------------------------------------
        This is an automatically generated e-mail. To reply, visit:
        https://reviews.apache.org/r/1027/#review985
        -----------------------------------------------------------

        On 2011-07-07 02:17:42, rajith attapattu wrote:

        -----------------------------------------------------------

        This is an automatically generated e-mail. To reply, visit:

        https://reviews.apache.org/r/1027/

        -----------------------------------------------------------

        (Updated 2011-07-07 02:17:42)

        Review request for qpid, Gordon Sim and Robbie Gemmell.

        Summary

        -------

        In order to verify the uniqueness of the client ID, a dummy session is created using client ID as it's name. This prevents any other connection from using same client ID as the session creation will fail. However this verification is switched off by default in order to preserve backwards compatibility. You need to use -Dqpid.verify_client_id=true switch verification on.

        In summary the following changes were made in order to support the above,

        1. A verifyClientID method was added to the connection delegates,

        2. AMQSession_0_10.java was modified to allow a name to be specified for the underlying AMQP session.

        3. A method was added to o.a.q.transport.Session.java to wait until the session state was changed from NEW to OPEN (or another state which triggers the error).

        4. Setter/Getter in Session.java to store/retrieve the SessionDetachCode and ConnectionDelegate to set the detach code.

        5. SessionDelegate to notify Session object when attached/dettached/closed is invoked.

        This addresses bug QPID-3269.

        https://issues.apache.org/jira/browse/QPID-3269

        Diffs

        -----

        http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/client/AMQConnection.java 1143628

        http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/client/AMQConnectionDelegate.java 1143628

        http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/client/AMQConnectionDelegate_0_10.java 1143628

        http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/client/AMQConnectionDelegate_8_0.java 1143628

        http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/client/AMQSession_0_10.java 1143628

        http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/client/XASessionImpl.java 1143628

        http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/common/src/main/java/org/apache/qpid/configuration/ClientProperties.java 1143628

        http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/common/src/main/java/org/apache/qpid/transport/ConnectionDelegate.java 1143628

        http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/common/src/main/java/org/apache/qpid/transport/Session.java 1143628

        http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/common/src/main/java/org/apache/qpid/transport/SessionDelegate.java 1143628

        Diff: https://reviews.apache.org/r/1027/diff

        Testing

        -------

        A patch containing a test will be attached to the JIRA shortly.

        Thanks,

        rajith

        Show
        jiraposter@reviews.apache.org added a comment - On 2011-07-07 12:14:51, Gordon Sim wrote: > http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/common/src/main/java/org/apache/qpid/transport/Session.java , line 1069 > < https://reviews.apache.org/r/1027/diff/1/?file=22094#file22094line1069 > > > Could you not have used sync() here instead of a new method? rajith attapattu wrote: I did wonder about this myself. But once a session is detached you can no longer sync on it right (from the broker side) ? (Bcos you need a valid session to sync on). Besides when the session is detached it's marked close on the client side, so sync wouldn't work anyways as we will throw a session closed exception. Gordon Sim wrote: The point would be though that if you are sync()ing on the client side and the session is detached, that already wakes you up and throws an exception, right? I don't understand the second sentence. However your new method is throwing a session closed exception as well. Gordon, my bad, you don't need a new method sync would suffice. I just tested using sync with a minor modification and it works. rajith ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/1027/#review985 ----------------------------------------------------------- On 2011-07-07 02:17:42, rajith attapattu wrote: ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/1027/ ----------------------------------------------------------- (Updated 2011-07-07 02:17:42) Review request for qpid, Gordon Sim and Robbie Gemmell. Summary ------- In order to verify the uniqueness of the client ID, a dummy session is created using client ID as it's name. This prevents any other connection from using same client ID as the session creation will fail. However this verification is switched off by default in order to preserve backwards compatibility. You need to use -Dqpid.verify_client_id=true switch verification on. In summary the following changes were made in order to support the above, 1. A verifyClientID method was added to the connection delegates, 2. AMQSession_0_10.java was modified to allow a name to be specified for the underlying AMQP session. 3. A method was added to o.a.q.transport.Session.java to wait until the session state was changed from NEW to OPEN (or another state which triggers the error). 4. Setter/Getter in Session.java to store/retrieve the SessionDetachCode and ConnectionDelegate to set the detach code. 5. SessionDelegate to notify Session object when attached/dettached/closed is invoked. This addresses bug QPID-3269 . https://issues.apache.org/jira/browse/QPID-3269 Diffs ----- http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/client/AMQConnection.java 1143628 http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/client/AMQConnectionDelegate.java 1143628 http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/client/AMQConnectionDelegate_0_10.java 1143628 http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/client/AMQConnectionDelegate_8_0.java 1143628 http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/client/AMQSession_0_10.java 1143628 http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/client/XASessionImpl.java 1143628 http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/common/src/main/java/org/apache/qpid/configuration/ClientProperties.java 1143628 http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/common/src/main/java/org/apache/qpid/transport/ConnectionDelegate.java 1143628 http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/common/src/main/java/org/apache/qpid/transport/Session.java 1143628 http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/common/src/main/java/org/apache/qpid/transport/SessionDelegate.java 1143628 Diff: https://reviews.apache.org/r/1027/diff Testing ------- A patch containing a test will be attached to the JIRA shortly. Thanks, rajith
        Hide
        jiraposter@reviews.apache.org added a comment -

        On 2011-07-07 12:14:51, Gordon Sim wrote:

        > http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/common/src/main/java/org/apache/qpid/transport/Session.java, line 1069

        > <https://reviews.apache.org/r/1027/diff/1/?file=22094#file22094line1069>

        >

        > Could you not have used sync() here instead of a new method?

        rajith attapattu wrote:

        I did wonder about this myself. But once a session is detached you can no longer sync on it right (from the broker side) ? (Bcos you need a valid session to sync on).

        Besides when the session is detached it's marked close on the client side, so sync wouldn't work anyways as we will throw a session closed exception.

        The point would be though that if you are sync()ing on the client side and the session is detached, that already wakes you up and throws an exception, right?

        I don't understand the second sentence. However your new method is throwing a session closed exception as well.

        • Gordon

        -----------------------------------------------------------
        This is an automatically generated e-mail. To reply, visit:
        https://reviews.apache.org/r/1027/#review985
        -----------------------------------------------------------

        On 2011-07-07 02:17:42, rajith attapattu wrote:

        -----------------------------------------------------------

        This is an automatically generated e-mail. To reply, visit:

        https://reviews.apache.org/r/1027/

        -----------------------------------------------------------

        (Updated 2011-07-07 02:17:42)

        Review request for qpid, Gordon Sim and Robbie Gemmell.

        Summary

        -------

        In order to verify the uniqueness of the client ID, a dummy session is created using client ID as it's name. This prevents any other connection from using same client ID as the session creation will fail. However this verification is switched off by default in order to preserve backwards compatibility. You need to use -Dqpid.verify_client_id=true switch verification on.

        In summary the following changes were made in order to support the above,

        1. A verifyClientID method was added to the connection delegates,

        2. AMQSession_0_10.java was modified to allow a name to be specified for the underlying AMQP session.

        3. A method was added to o.a.q.transport.Session.java to wait until the session state was changed from NEW to OPEN (or another state which triggers the error).

        4. Setter/Getter in Session.java to store/retrieve the SessionDetachCode and ConnectionDelegate to set the detach code.

        5. SessionDelegate to notify Session object when attached/dettached/closed is invoked.

        This addresses bug QPID-3269.

        https://issues.apache.org/jira/browse/QPID-3269

        Diffs

        -----

        http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/client/AMQConnection.java 1143628

        http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/client/AMQConnectionDelegate.java 1143628

        http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/client/AMQConnectionDelegate_0_10.java 1143628

        http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/client/AMQConnectionDelegate_8_0.java 1143628

        http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/client/AMQSession_0_10.java 1143628

        http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/client/XASessionImpl.java 1143628

        http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/common/src/main/java/org/apache/qpid/configuration/ClientProperties.java 1143628

        http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/common/src/main/java/org/apache/qpid/transport/ConnectionDelegate.java 1143628

        http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/common/src/main/java/org/apache/qpid/transport/Session.java 1143628

        http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/common/src/main/java/org/apache/qpid/transport/SessionDelegate.java 1143628

        Diff: https://reviews.apache.org/r/1027/diff

        Testing

        -------

        A patch containing a test will be attached to the JIRA shortly.

        Thanks,

        rajith

        Show
        jiraposter@reviews.apache.org added a comment - On 2011-07-07 12:14:51, Gordon Sim wrote: > http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/common/src/main/java/org/apache/qpid/transport/Session.java , line 1069 > < https://reviews.apache.org/r/1027/diff/1/?file=22094#file22094line1069 > > > Could you not have used sync() here instead of a new method? rajith attapattu wrote: I did wonder about this myself. But once a session is detached you can no longer sync on it right (from the broker side) ? (Bcos you need a valid session to sync on). Besides when the session is detached it's marked close on the client side, so sync wouldn't work anyways as we will throw a session closed exception. The point would be though that if you are sync()ing on the client side and the session is detached, that already wakes you up and throws an exception, right? I don't understand the second sentence. However your new method is throwing a session closed exception as well. Gordon ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/1027/#review985 ----------------------------------------------------------- On 2011-07-07 02:17:42, rajith attapattu wrote: ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/1027/ ----------------------------------------------------------- (Updated 2011-07-07 02:17:42) Review request for qpid, Gordon Sim and Robbie Gemmell. Summary ------- In order to verify the uniqueness of the client ID, a dummy session is created using client ID as it's name. This prevents any other connection from using same client ID as the session creation will fail. However this verification is switched off by default in order to preserve backwards compatibility. You need to use -Dqpid.verify_client_id=true switch verification on. In summary the following changes were made in order to support the above, 1. A verifyClientID method was added to the connection delegates, 2. AMQSession_0_10.java was modified to allow a name to be specified for the underlying AMQP session. 3. A method was added to o.a.q.transport.Session.java to wait until the session state was changed from NEW to OPEN (or another state which triggers the error). 4. Setter/Getter in Session.java to store/retrieve the SessionDetachCode and ConnectionDelegate to set the detach code. 5. SessionDelegate to notify Session object when attached/dettached/closed is invoked. This addresses bug QPID-3269 . https://issues.apache.org/jira/browse/QPID-3269 Diffs ----- http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/client/AMQConnection.java 1143628 http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/client/AMQConnectionDelegate.java 1143628 http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/client/AMQConnectionDelegate_0_10.java 1143628 http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/client/AMQConnectionDelegate_8_0.java 1143628 http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/client/AMQSession_0_10.java 1143628 http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/client/XASessionImpl.java 1143628 http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/common/src/main/java/org/apache/qpid/configuration/ClientProperties.java 1143628 http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/common/src/main/java/org/apache/qpid/transport/ConnectionDelegate.java 1143628 http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/common/src/main/java/org/apache/qpid/transport/Session.java 1143628 http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/common/src/main/java/org/apache/qpid/transport/SessionDelegate.java 1143628 Diff: https://reviews.apache.org/r/1027/diff Testing ------- A patch containing a test will be attached to the JIRA shortly. Thanks, rajith
        Hide
        jiraposter@reviews.apache.org added a comment -

        On 2011-07-07 12:14:51, Gordon Sim wrote:

        > http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/client/AMQConnectionDelegate_0_10.java, line 472

        > <https://reviews.apache.org/r/1027/diff/1/?file=22088#file22088line472>

        >

        > What if any is the implication for failover?

        >

        > The session object is scoped to this method. Is that intended? There is a reference to it held somewhere else? Will it be cleaned up correctly?

        Good question: There is a reference to it held within the respective Connection object. The session is created using the createSession method in the same class, which registers this session with the connection. Therefore it will be recreated after failover and cleaned up upon connection close.

        On 2011-07-07 12:14:51, Gordon Sim wrote:

        > http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/common/src/main/java/org/apache/qpid/transport/Session.java, line 1069

        > <https://reviews.apache.org/r/1027/diff/1/?file=22094#file22094line1069>

        >

        > Could you not have used sync() here instead of a new method?

        I did wonder about this myself. But once a session is detached you can no longer sync on it right (from the broker side) ? (Bcos you need a valid session to sync on).
        Besides when the session is detached it's marked close on the client side, so sync wouldn't work anyways as we will throw a session closed exception.

        • rajith

        -----------------------------------------------------------
        This is an automatically generated e-mail. To reply, visit:
        https://reviews.apache.org/r/1027/#review985
        -----------------------------------------------------------

        On 2011-07-07 02:17:42, rajith attapattu wrote:

        -----------------------------------------------------------

        This is an automatically generated e-mail. To reply, visit:

        https://reviews.apache.org/r/1027/

        -----------------------------------------------------------

        (Updated 2011-07-07 02:17:42)

        Review request for qpid, Gordon Sim and Robbie Gemmell.

        Summary

        -------

        In order to verify the uniqueness of the client ID, a dummy session is created using client ID as it's name. This prevents any other connection from using same client ID as the session creation will fail. However this verification is switched off by default in order to preserve backwards compatibility. You need to use -Dqpid.verify_client_id=true switch verification on.

        In summary the following changes were made in order to support the above,

        1. A verifyClientID method was added to the connection delegates,

        2. AMQSession_0_10.java was modified to allow a name to be specified for the underlying AMQP session.

        3. A method was added to o.a.q.transport.Session.java to wait until the session state was changed from NEW to OPEN (or another state which triggers the error).

        4. Setter/Getter in Session.java to store/retrieve the SessionDetachCode and ConnectionDelegate to set the detach code.

        5. SessionDelegate to notify Session object when attached/dettached/closed is invoked.

        This addresses bug QPID-3269.

        https://issues.apache.org/jira/browse/QPID-3269

        Diffs

        -----

        http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/client/AMQConnection.java 1143628

        http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/client/AMQConnectionDelegate.java 1143628

        http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/client/AMQConnectionDelegate_0_10.java 1143628

        http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/client/AMQConnectionDelegate_8_0.java 1143628

        http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/client/AMQSession_0_10.java 1143628

        http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/client/XASessionImpl.java 1143628

        http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/common/src/main/java/org/apache/qpid/configuration/ClientProperties.java 1143628

        http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/common/src/main/java/org/apache/qpid/transport/ConnectionDelegate.java 1143628

        http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/common/src/main/java/org/apache/qpid/transport/Session.java 1143628

        http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/common/src/main/java/org/apache/qpid/transport/SessionDelegate.java 1143628

        Diff: https://reviews.apache.org/r/1027/diff

        Testing

        -------

        A patch containing a test will be attached to the JIRA shortly.

        Thanks,

        rajith

        Show
        jiraposter@reviews.apache.org added a comment - On 2011-07-07 12:14:51, Gordon Sim wrote: > http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/client/AMQConnectionDelegate_0_10.java , line 472 > < https://reviews.apache.org/r/1027/diff/1/?file=22088#file22088line472 > > > What if any is the implication for failover? > > The session object is scoped to this method. Is that intended? There is a reference to it held somewhere else? Will it be cleaned up correctly? Good question: There is a reference to it held within the respective Connection object. The session is created using the createSession method in the same class, which registers this session with the connection. Therefore it will be recreated after failover and cleaned up upon connection close. On 2011-07-07 12:14:51, Gordon Sim wrote: > http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/common/src/main/java/org/apache/qpid/transport/Session.java , line 1069 > < https://reviews.apache.org/r/1027/diff/1/?file=22094#file22094line1069 > > > Could you not have used sync() here instead of a new method? I did wonder about this myself. But once a session is detached you can no longer sync on it right (from the broker side) ? (Bcos you need a valid session to sync on). Besides when the session is detached it's marked close on the client side, so sync wouldn't work anyways as we will throw a session closed exception. rajith ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/1027/#review985 ----------------------------------------------------------- On 2011-07-07 02:17:42, rajith attapattu wrote: ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/1027/ ----------------------------------------------------------- (Updated 2011-07-07 02:17:42) Review request for qpid, Gordon Sim and Robbie Gemmell. Summary ------- In order to verify the uniqueness of the client ID, a dummy session is created using client ID as it's name. This prevents any other connection from using same client ID as the session creation will fail. However this verification is switched off by default in order to preserve backwards compatibility. You need to use -Dqpid.verify_client_id=true switch verification on. In summary the following changes were made in order to support the above, 1. A verifyClientID method was added to the connection delegates, 2. AMQSession_0_10.java was modified to allow a name to be specified for the underlying AMQP session. 3. A method was added to o.a.q.transport.Session.java to wait until the session state was changed from NEW to OPEN (or another state which triggers the error). 4. Setter/Getter in Session.java to store/retrieve the SessionDetachCode and ConnectionDelegate to set the detach code. 5. SessionDelegate to notify Session object when attached/dettached/closed is invoked. This addresses bug QPID-3269 . https://issues.apache.org/jira/browse/QPID-3269 Diffs ----- http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/client/AMQConnection.java 1143628 http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/client/AMQConnectionDelegate.java 1143628 http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/client/AMQConnectionDelegate_0_10.java 1143628 http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/client/AMQConnectionDelegate_8_0.java 1143628 http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/client/AMQSession_0_10.java 1143628 http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/client/XASessionImpl.java 1143628 http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/common/src/main/java/org/apache/qpid/configuration/ClientProperties.java 1143628 http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/common/src/main/java/org/apache/qpid/transport/ConnectionDelegate.java 1143628 http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/common/src/main/java/org/apache/qpid/transport/Session.java 1143628 http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/common/src/main/java/org/apache/qpid/transport/SessionDelegate.java 1143628 Diff: https://reviews.apache.org/r/1027/diff Testing ------- A patch containing a test will be attached to the JIRA shortly. Thanks, rajith
        Hide
        jiraposter@reviews.apache.org added a comment -

        -----------------------------------------------------------
        This is an automatically generated e-mail. To reply, visit:
        https://reviews.apache.org/r/1027/#review985
        -----------------------------------------------------------

        Couple of minor points below. In general looks ok to me though I am not as familiar with the codebase as a whole as I should be.

        http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/client/AMQConnectionDelegate_0_10.java
        <https://reviews.apache.org/r/1027/#comment2051>

        What if any is the implication for failover?

        The session object is scoped to this method. Is that intended? There is a reference to it held somewhere else? Will it be cleaned up correctly?

        http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/common/src/main/java/org/apache/qpid/transport/Session.java
        <https://reviews.apache.org/r/1027/#comment2050>

        Could you not have used sync() here instead of a new method?

        • Gordon

        On 2011-07-07 02:17:42, rajith attapattu wrote:

        -----------------------------------------------------------

        This is an automatically generated e-mail. To reply, visit:

        https://reviews.apache.org/r/1027/

        -----------------------------------------------------------

        (Updated 2011-07-07 02:17:42)

        Review request for qpid, Gordon Sim and Robbie Gemmell.

        Summary

        -------

        In order to verify the uniqueness of the client ID, a dummy session is created using client ID as it's name. This prevents any other connection from using same client ID as the session creation will fail. However this verification is switched off by default in order to preserve backwards compatibility. You need to use -Dqpid.verify_client_id=true switch verification on.

        In summary the following changes were made in order to support the above,

        1. A verifyClientID method was added to the connection delegates,

        2. AMQSession_0_10.java was modified to allow a name to be specified for the underlying AMQP session.

        3. A method was added to o.a.q.transport.Session.java to wait until the session state was changed from NEW to OPEN (or another state which triggers the error).

        4. Setter/Getter in Session.java to store/retrieve the SessionDetachCode and ConnectionDelegate to set the detach code.

        5. SessionDelegate to notify Session object when attached/dettached/closed is invoked.

        This addresses bug QPID-3269.

        https://issues.apache.org/jira/browse/QPID-3269

        Diffs

        -----

        http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/client/AMQConnection.java 1143628

        http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/client/AMQConnectionDelegate.java 1143628

        http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/client/AMQConnectionDelegate_0_10.java 1143628

        http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/client/AMQConnectionDelegate_8_0.java 1143628

        http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/client/AMQSession_0_10.java 1143628

        http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/client/XASessionImpl.java 1143628

        http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/common/src/main/java/org/apache/qpid/configuration/ClientProperties.java 1143628

        http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/common/src/main/java/org/apache/qpid/transport/ConnectionDelegate.java 1143628

        http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/common/src/main/java/org/apache/qpid/transport/Session.java 1143628

        http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/common/src/main/java/org/apache/qpid/transport/SessionDelegate.java 1143628

        Diff: https://reviews.apache.org/r/1027/diff

        Testing

        -------

        A patch containing a test will be attached to the JIRA shortly.

        Thanks,

        rajith

        Show
        jiraposter@reviews.apache.org added a comment - ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/1027/#review985 ----------------------------------------------------------- Couple of minor points below. In general looks ok to me though I am not as familiar with the codebase as a whole as I should be. http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/client/AMQConnectionDelegate_0_10.java < https://reviews.apache.org/r/1027/#comment2051 > What if any is the implication for failover? The session object is scoped to this method. Is that intended? There is a reference to it held somewhere else? Will it be cleaned up correctly? http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/common/src/main/java/org/apache/qpid/transport/Session.java < https://reviews.apache.org/r/1027/#comment2050 > Could you not have used sync() here instead of a new method? Gordon On 2011-07-07 02:17:42, rajith attapattu wrote: ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/1027/ ----------------------------------------------------------- (Updated 2011-07-07 02:17:42) Review request for qpid, Gordon Sim and Robbie Gemmell. Summary ------- In order to verify the uniqueness of the client ID, a dummy session is created using client ID as it's name. This prevents any other connection from using same client ID as the session creation will fail. However this verification is switched off by default in order to preserve backwards compatibility. You need to use -Dqpid.verify_client_id=true switch verification on. In summary the following changes were made in order to support the above, 1. A verifyClientID method was added to the connection delegates, 2. AMQSession_0_10.java was modified to allow a name to be specified for the underlying AMQP session. 3. A method was added to o.a.q.transport.Session.java to wait until the session state was changed from NEW to OPEN (or another state which triggers the error). 4. Setter/Getter in Session.java to store/retrieve the SessionDetachCode and ConnectionDelegate to set the detach code. 5. SessionDelegate to notify Session object when attached/dettached/closed is invoked. This addresses bug QPID-3269 . https://issues.apache.org/jira/browse/QPID-3269 Diffs ----- http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/client/AMQConnection.java 1143628 http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/client/AMQConnectionDelegate.java 1143628 http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/client/AMQConnectionDelegate_0_10.java 1143628 http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/client/AMQConnectionDelegate_8_0.java 1143628 http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/client/AMQSession_0_10.java 1143628 http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/client/XASessionImpl.java 1143628 http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/common/src/main/java/org/apache/qpid/configuration/ClientProperties.java 1143628 http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/common/src/main/java/org/apache/qpid/transport/ConnectionDelegate.java 1143628 http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/common/src/main/java/org/apache/qpid/transport/Session.java 1143628 http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/common/src/main/java/org/apache/qpid/transport/SessionDelegate.java 1143628 Diff: https://reviews.apache.org/r/1027/diff Testing ------- A patch containing a test will be attached to the JIRA shortly. Thanks, rajith
        Hide
        jiraposter@reviews.apache.org added a comment -

        -----------------------------------------------------------
        This is an automatically generated e-mail. To reply, visit:
        https://reviews.apache.org/r/1027/
        -----------------------------------------------------------

        Review request for qpid, Gordon Sim and Robbie Gemmell.

        Summary
        -------

        In order to verify the uniqueness of the client ID, a dummy session is created using client ID as it's name. This prevents any other connection from using same client ID as the session creation will fail. However this verification is switched off by default in order to preserve backwards compatibility. You need to use -Dqpid.verify_client_id=true switch verification on.

        In summary the following changes were made in order to support the above,
        1. A verifyClientID method was added to the connection delegates,
        2. AMQSession_0_10.java was modified to allow a name to be specified for the underlying AMQP session.
        3. A method was added to o.a.q.transport.Session.java to wait until the session state was changed from NEW to OPEN (or another state which triggers the error).
        4. Setter/Getter in Session.java to store/retrieve the SessionDetachCode and ConnectionDelegate to set the detach code.
        5. SessionDelegate to notify Session object when attached/dettached/closed is invoked.

        This addresses bug QPID-3269.
        https://issues.apache.org/jira/browse/QPID-3269

        Diffs


        http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/client/AMQConnection.java 1143628
        http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/client/AMQConnectionDelegate.java 1143628
        http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/client/AMQConnectionDelegate_0_10.java 1143628
        http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/client/AMQConnectionDelegate_8_0.java 1143628
        http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/client/AMQSession_0_10.java 1143628
        http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/client/XASessionImpl.java 1143628
        http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/common/src/main/java/org/apache/qpid/configuration/ClientProperties.java 1143628
        http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/common/src/main/java/org/apache/qpid/transport/ConnectionDelegate.java 1143628
        http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/common/src/main/java/org/apache/qpid/transport/Session.java 1143628
        http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/common/src/main/java/org/apache/qpid/transport/SessionDelegate.java 1143628

        Diff: https://reviews.apache.org/r/1027/diff

        Testing
        -------

        A patch containing a test will be attached to the JIRA shortly.

        Thanks,

        rajith

        Show
        jiraposter@reviews.apache.org added a comment - ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/1027/ ----------------------------------------------------------- Review request for qpid, Gordon Sim and Robbie Gemmell. Summary ------- In order to verify the uniqueness of the client ID, a dummy session is created using client ID as it's name. This prevents any other connection from using same client ID as the session creation will fail. However this verification is switched off by default in order to preserve backwards compatibility. You need to use -Dqpid.verify_client_id=true switch verification on. In summary the following changes were made in order to support the above, 1. A verifyClientID method was added to the connection delegates, 2. AMQSession_0_10.java was modified to allow a name to be specified for the underlying AMQP session. 3. A method was added to o.a.q.transport.Session.java to wait until the session state was changed from NEW to OPEN (or another state which triggers the error). 4. Setter/Getter in Session.java to store/retrieve the SessionDetachCode and ConnectionDelegate to set the detach code. 5. SessionDelegate to notify Session object when attached/dettached/closed is invoked. This addresses bug QPID-3269 . https://issues.apache.org/jira/browse/QPID-3269 Diffs http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/client/AMQConnection.java 1143628 http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/client/AMQConnectionDelegate.java 1143628 http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/client/AMQConnectionDelegate_0_10.java 1143628 http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/client/AMQConnectionDelegate_8_0.java 1143628 http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/client/AMQSession_0_10.java 1143628 http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/client/XASessionImpl.java 1143628 http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/common/src/main/java/org/apache/qpid/configuration/ClientProperties.java 1143628 http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/common/src/main/java/org/apache/qpid/transport/ConnectionDelegate.java 1143628 http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/common/src/main/java/org/apache/qpid/transport/Session.java 1143628 http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/common/src/main/java/org/apache/qpid/transport/SessionDelegate.java 1143628 Diff: https://reviews.apache.org/r/1027/diff Testing ------- A patch containing a test will be attached to the JIRA shortly. Thanks, rajith

          People

          • Assignee:
            Rajith Attapattu
            Reporter:
            Rajith Attapattu
          • Votes:
            0 Vote for this issue
            Watchers:
            0 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development