[GitHub] activemq-artemis pull request #1202: ARTEMIS-1111 Avoid deadlock on AMQP del...

Previous Topic Next Topic
 
classic Classic list List threaded Threaded
7 messages Options
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[GitHub] activemq-artemis pull request #1202: ARTEMIS-1111 Avoid deadlock on AMQP del...

asfgit
GitHub user mtaylor opened a pull request:

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

    ARTEMIS-1111 Avoid deadlock on AMQP delivery during close

    I wanted to avoid the possibility of adding more race/lock conditions by introducing new Executors/Threads when dealing with the situation described by ARTEMIS-1111.  Instead I've upgraded the connection lock to a re-entrant lock.  Instead of the consumer delivery thread blocking forever when the ProtonHandler lock is held by the close() event, it does a tryLock(), then checks to see if the link is closed.  If so it will exit.  If not it will retry.

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

    $ git pull https://github.com/mtaylor/activemq-artemis ARTEMIS-1111

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

    https://github.com/apache/activemq-artemis/pull/1202.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 #1202
   
----
commit 153053f8c71585bdfd66c353269a9e87aa1942f7
Author: Martyn Taylor <[hidden email]>
Date:   2017-04-12T13:38:06Z

    ARTEMIS-1111 Avoid deadlock on AMQP delivery during close

----


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at [hidden email] or file a JIRA ticket
with INFRA.
---
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[GitHub] activemq-artemis issue #1202: ARTEMIS-1111 Avoid deadlock on AMQP delivery d...

asfgit
Github user mtaylor commented on the issue:

    https://github.com/apache/activemq-artemis/pull/1202
 
    @clebertsuconic Slightly different solution to what we had previously discussed.  You might want to take a look.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at [hidden email] or file a JIRA ticket
with INFRA.
---
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[GitHub] activemq-artemis pull request #1202: ARTEMIS-1111 Avoid deadlock on AMQP del...

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

    https://github.com/apache/activemq-artemis/pull/1202#discussion_r111171950
 
    --- Diff: artemis-protocols/artemis-amqp-protocol/src/main/java/org/apache/activemq/artemis/protocol/amqp/proton/AMQPSessionContext.java ---
    @@ -161,18 +164,25 @@ public void addSender(Sender sender) throws Exception {
           try {
              protonSender.initialise();
              senders.put(sender, protonSender);
    -         serverSenders.put(protonSender.getBrokerConsumer(), protonSender);
    +         ser\nverSenders.put(protonSender.getBrokerConsumer(), protonSender);
    --- End diff --
   
    typo here


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at [hidden email] or file a JIRA ticket
with INFRA.
---
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[GitHub] activemq-artemis pull request #1202: ARTEMIS-1111 Avoid deadlock on AMQP del...

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

    https://github.com/apache/activemq-artemis/pull/1202#discussion_r111172287
 
    --- Diff: artemis-protocols/artemis-amqp-protocol/src/main/java/org/apache/activemq/artemis/protocol/amqp/proton/ProtonServerSenderContext.java ---
    @@ -617,10 +630,15 @@ public int deliverMessage(MessageReference messageReference, int deliveryCount)
     
              int size = nettyBuffer.writerIndex();
     
    -         synchronized (connection.getLock()) {
    -            if (sender.getLocalState() == EndpointState.CLOSED) {
    +         while (!connection.getLock().tryLock(1, TimeUnit.SECONDS)) {
    +            if (closed || sender.getLocalState() == EndpointState.CLOSED) {
    +               // If we're waiting on the connection lock, the link might be in the process of closing.  If this happens
    +               // we return.
                    return 0;
                 }
    --- End diff --
   
    else... log.debug please?


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at [hidden email] or file a JIRA ticket
with INFRA.
---
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[GitHub] activemq-artemis pull request #1202: ARTEMIS-1111 Avoid deadlock on AMQP del...

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

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


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at [hidden email] or file a JIRA ticket
with INFRA.
---
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[GitHub] activemq-artemis pull request #1202: ARTEMIS-1111 Avoid deadlock on AMQP del...

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

    https://github.com/apache/activemq-artemis/pull/1202#discussion_r111669868
 
    --- Diff: tests/integration-tests/src/test/java/org/apache/activemq/artemis/tests/integration/amqp/ProtonTest.java ---
    @@ -1583,6 +1583,45 @@ public void testSimpleText() throws Throwable {
        }
     
        @Test
    +   public void testTimedOutWaitingForWriteLogOnConsumer() throws Throwable {
    --- End diff --
   
    test with the exact same name is already on line 534; one of them should be renamed


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at [hidden email] or file a JIRA ticket
with INFRA.
---
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[GitHub] activemq-artemis pull request #1202: ARTEMIS-1111 Avoid deadlock on AMQP del...

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

    https://github.com/apache/activemq-artemis/pull/1202#discussion_r111670014
 
    --- Diff: tests/integration-tests/src/test/java/org/apache/activemq/artemis/tests/integration/amqp/ProtonTest.java ---
    @@ -1583,6 +1583,45 @@ public void testSimpleText() throws Throwable {
        }
     
        @Test
    +   public void testTimedOutWaitingForWriteLogOnConsumer() throws Throwable {
    --- End diff --
   
    ugh, never mind, my mistake, not sure what I saw there...


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at [hidden email] or file a JIRA ticket
with INFRA.
---
Loading...