[GitHub] activemq-artemis pull request #2448: ARTEMIS-2189 allow deleting temp dest w...

classic Classic list List threaded Threaded
9 messages Options
Reply | Threaded
Open this post in threaded view
|

[GitHub] activemq-artemis pull request #2448: ARTEMIS-2189 allow deleting temp dest w...

rstancel
GitHub user jbertram opened a pull request:

    https://github.com/apache/activemq-artemis/pull/2448

    ARTEMIS-2189 allow deleting temp dest when session is closed

   

You can merge this pull request into a Git repository by running:

    $ git pull https://github.com/jbertram/activemq-artemis ARTEMIS-2189

Alternatively you can review and apply these changes as the patch at:

    https://github.com/apache/activemq-artemis/pull/2448.patch

To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:

    This closes #2448
   
----
commit ff4176573105fbefb57ec23f2a446f434437ca79
Author: Justin Bertram <jbertram@...>
Date:   2018-12-03T19:44:32Z

    ARTEMIS-2189 allow deleting temp dest when session is closed

----


---
Reply | Threaded
Open this post in threaded view
|

[GitHub] activemq-artemis pull request #2448: ARTEMIS-2189 allow deleting temp dest w...

rstancel
Github user michaelandrepearce commented on a diff in the pull request:

    https://github.com/apache/activemq-artemis/pull/2448#discussion_r238413260
 
    --- Diff: tests/jms-tests/src/test/java/org/apache/activemq/artemis/jms/tests/TemporaryDestinationTest.java ---
    @@ -265,6 +266,126 @@ public void testTemporaryQueueDeleted() throws Exception {
           }
        }
     
    +   @Test
    +   public void testTemporaryQueueDeletedAfterSessionClosed() throws Exception {
    --- End diff --
   
    Can the behaviour be checked with amqp and openwire to check impl behaviour is the same


---
Reply | Threaded
Open this post in threaded view
|

[GitHub] activemq-artemis pull request #2448: ARTEMIS-2189 allow deleting temp dest w...

rstancel
In reply to this post by rstancel
Github user tabish121 commented on a diff in the pull request:

    https://github.com/apache/activemq-artemis/pull/2448#discussion_r238416943
 
    --- Diff: tests/jms-tests/src/test/java/org/apache/activemq/artemis/jms/tests/TemporaryDestinationTest.java ---
    @@ -265,6 +266,126 @@ public void testTemporaryQueueDeleted() throws Exception {
           }
        }
     
    +   @Test
    +   public void testTemporaryQueueDeletedAfterSessionClosed() throws Exception {
    --- End diff --
   
    For AMQP the handling of dynamic nodes is tested here:
    https://github.com/apache/activemq-artemis/blob/master/tests/integration-tests/src/test/java/org/apache/activemq/artemis/tests/integration/amqp/AmqpTempDestinationTest.java
    And here:
    https://github.com/apache/activemq-artemis/blob/master/tests/integration-tests/src/test/java/org/apache/activemq/artemis/tests/integration/amqp/JMSTemporaryDestinationTest.java
   
    For OpenWire you'd want to look for a test of the RemoveInfo command that carries a DestintionInfo object.
   
    Other tests that are specifically targeted the JMS spec (or other) compliant behaviour of a client really belong to the project that maintains the client in question.


---
Reply | Threaded
Open this post in threaded view
|

[GitHub] activemq-artemis pull request #2448: ARTEMIS-2189 allow deleting temp dest w...

rstancel
In reply to this post by rstancel
Github user jbertram commented on a diff in the pull request:

    https://github.com/apache/activemq-artemis/pull/2448#discussion_r238422596
 
    --- Diff: tests/jms-tests/src/test/java/org/apache/activemq/artemis/jms/tests/TemporaryDestinationTest.java ---
    @@ -265,6 +266,126 @@ public void testTemporaryQueueDeleted() throws Exception {
           }
        }
     
    +   @Test
    +   public void testTemporaryQueueDeletedAfterSessionClosed() throws Exception {
    --- End diff --
   
    The issue here is not with the broker but with the core client. Therefore, I'm not sure it makes sense to test other clients on this particular use-case.


---
Reply | Threaded
Open this post in threaded view
|

[GitHub] activemq-artemis pull request #2448: ARTEMIS-2189 allow deleting temp dest w...

rstancel
In reply to this post by rstancel
Github user tabish121 commented on a diff in the pull request:

    https://github.com/apache/activemq-artemis/pull/2448#discussion_r238785616
 
    --- Diff: artemis-jms-client/src/main/java/org/apache/activemq/artemis/jms/client/ActiveMQDestination.java ---
    @@ -386,14 +386,27 @@ public void setSimpleAddress(SimpleString address) {
     
        public void delete() throws JMSException {
           if (session != null) {
    +         ActiveMQSession sessionToUse = session;
    +         boolean temporary = false;
              if (session.getCoreSession().isClosed()) {
    -            // Temporary queues will be deleted when the connection is closed.. nothing to be done then!
    -            return;
    +            /**
    --- End diff --
   
    @jbertram This code is still a bit racey in that the session could become closed right after checking that it isn't and thereby still allow a violation in what the spec says should be possible.  It might make more sense to just use the approach of creating a session for this operation each time given this isn't something people are going to be doing a great deal and so doesn't need to be highly performant.


---
Reply | Threaded
Open this post in threaded view
|

[GitHub] activemq-artemis pull request #2448: ARTEMIS-2189 allow deleting temp dest w...

rstancel
In reply to this post by rstancel
Github user jbertram commented on a diff in the pull request:

    https://github.com/apache/activemq-artemis/pull/2448#discussion_r238910671
 
    --- Diff: artemis-jms-client/src/main/java/org/apache/activemq/artemis/jms/client/ActiveMQDestination.java ---
    @@ -386,14 +386,27 @@ public void setSimpleAddress(SimpleString address) {
     
        public void delete() throws JMSException {
           if (session != null) {
    +         ActiveMQSession sessionToUse = session;
    +         boolean temporary = false;
              if (session.getCoreSession().isClosed()) {
    -            // Temporary queues will be deleted when the connection is closed.. nothing to be done then!
    -            return;
    +            /**
    --- End diff --
   
    Fair enough. I pushed an update.


---
Reply | Threaded
Open this post in threaded view
|

[GitHub] activemq-artemis pull request #2448: ARTEMIS-2189 allow deleting temp dest w...

rstancel
In reply to this post by rstancel
Github user franz1981 commented on a diff in the pull request:

    https://github.com/apache/activemq-artemis/pull/2448#discussion_r239482834
 
    --- Diff: artemis-jms-client/src/main/java/org/apache/activemq/artemis/jms/client/ActiveMQDestination.java ---
    @@ -386,14 +386,27 @@ public void setSimpleAddress(SimpleString address) {
     
        public void delete() throws JMSException {
           if (session != null) {
    +         ActiveMQSession sessionToUse = session;
    +         boolean temporary = false;
              if (session.getCoreSession().isClosed()) {
    -            // Temporary queues will be deleted when the connection is closed.. nothing to be done then!
    -            return;
    +            /**
    --- End diff --
   
    @tabish121 The updated version is fine mate? If is fine I will merge it :+1:


---
Reply | Threaded
Open this post in threaded view
|

[GitHub] activemq-artemis pull request #2448: ARTEMIS-2189 allow deleting temp dest w...

rstancel
In reply to this post by rstancel
Github user tabish121 commented on a diff in the pull request:

    https://github.com/apache/activemq-artemis/pull/2448#discussion_r239486511
 
    --- Diff: artemis-jms-client/src/main/java/org/apache/activemq/artemis/jms/client/ActiveMQDestination.java ---
    @@ -386,14 +386,27 @@ public void setSimpleAddress(SimpleString address) {
     
        public void delete() throws JMSException {
           if (session != null) {
    +         ActiveMQSession sessionToUse = session;
    +         boolean temporary = false;
              if (session.getCoreSession().isClosed()) {
    -            // Temporary queues will be deleted when the connection is closed.. nothing to be done then!
    -            return;
    +            /**
    --- End diff --
   
    Looks ok to me now, shouldn't be racing with session close now.


---
Reply | Threaded
Open this post in threaded view
|

[GitHub] activemq-artemis pull request #2448: ARTEMIS-2189 allow deleting temp dest w...

rstancel
In reply to this post by rstancel
Github user asfgit closed the pull request at:

    https://github.com/apache/activemq-artemis/pull/2448


---