[ 
https://issues.apache.org/jira/browse/ARTEMIS-3622?focusedWorklogId=920214&page=com.atlassian.jira.plugin.system.issuetabpanels:worklog-tabpanel#worklog-920214
 ]

ASF GitHub Bot logged work on ARTEMIS-3622:
-------------------------------------------

                Author: ASF GitHub Bot
            Created on: 21/May/24 13:33
            Start Date: 21/May/24 13:33
    Worklog Time Spent: 10m 
      Work Description: jbertram commented on code in PR #4909:
URL: https://github.com/apache/activemq-artemis/pull/4909#discussion_r1608344664


##########
artemis-protocols/artemis-mqtt-protocol/src/main/java/org/apache/activemq/artemis/core/protocol/mqtt/MQTTConnectionManager.java:
##########
@@ -176,33 +174,27 @@ ServerSessionImpl createServerSession(String username, 
String password, String v
       return (ServerSessionImpl) serverSession;
    }
 
-   void disconnect(boolean failure) {
+   synchronized void disconnect(boolean failure) {
       if (session == null || session.getStopped()) {
          return;
       }
 
-      synchronized (session.getState()) {

Review Comment:
   I've looked through it carefully and I don't see any potential problems. The 
relevant changes to the state are performed by `connect` and `disconnect` which 
are themselves now use `synchronized`.





Issue Time Tracking
-------------------

    Worklog Id:     (was: 920214)
    Time Spent: 0.5h  (was: 20m)

> MQTT can deadlock on client connection / disconnection
> ------------------------------------------------------
>
>                 Key: ARTEMIS-3622
>                 URL: https://issues.apache.org/jira/browse/ARTEMIS-3622
>             Project: ActiveMQ Artemis
>          Issue Type: Bug
>          Components: MQTT
>    Affects Versions: 2.19.0
>         Environment: Using the latest java 17 and artemis 2.19 but looking at 
> the code, it should affect 2.20 as well.
>            Reporter: Marcelo Takeshi Fukushima
>            Assignee: Justin Bertram
>            Priority: Major
>          Time Spent: 0.5h
>  Remaining Estimate: 0h
>
> It seems that the {{MQTTProtocolHandler}} and {{MQTTConnectionManager}} are 
> on a racing condition and can deadlock themselves on misbehaving clients. I'm 
> including the relevant stack trace (ignore thread 11 that is just waiting for 
> the lock).
> Looking at the relevant code, it seems that the clean-up thread (88 on the 
> {{{}MQTTFailureListener{}}}) starts cleaning up the session state and then 
> the session, but when {{MQTTSession.stop}} calls 
> {{{}MQTTSessionState.clear{}}}, the session state is no longer the same (a 
> racy connection has replaced the session state with a brand new under the 
> same client-id).
> I think the methods connect and disconnect on the {{MQTTConnectionManager}} 
> could be marked as synchronized as a whole, to prevent racy connects / 
> disconnects (but since I don't know all the ins and outs of the code, you 
> guys might have a better fix).
> {noformat}
> Found one Java-level deadlock:
> =============================
> "Thread-11 
> (ActiveMQ-server-org.apache.activemq.artemis.core.server.impl.ActiveMQServerImpl$6@640f11a1)":
>   waiting to lock monitor 0x00007f6d003368c0 (object 0x000000045f29f240, a 
> org.apache.activemq.artemis.core.protocol.mqtt.MQTTSessionState),
>   which is held by "Thread-24 
> (ActiveMQ-server-org.apache.activemq.artemis.core.server.impl.ActiveMQServerImpl$6@640f11a1)"
> "Thread-24 
> (ActiveMQ-server-org.apache.activemq.artemis.core.server.impl.ActiveMQServerImpl$6@640f11a1)":
>   waiting to lock monitor 0x00007f6d00336a80 (object 0x000000045f2a1068, a 
> org.apache.activemq.artemis.core.protocol.mqtt.MQTTSession),
>   which is held by "Thread-88 
> (ActiveMQ-remoting-threads-ActiveMQServerImpl::name=0.0.0.0-212232499)"
> "Thread-88 
> (ActiveMQ-remoting-threads-ActiveMQServerImpl::name=0.0.0.0-212232499)":
>   waiting to lock monitor 0x00007f6d003368c0 (object 0x000000045f29f240, a 
> org.apache.activemq.artemis.core.protocol.mqtt.MQTTSessionState),
>   which is held by "Thread-24 
> (ActiveMQ-server-org.apache.activemq.artemis.core.server.impl.ActiveMQServerImpl$6@640f11a1)"
> Java stack information for the threads listed above:
> ===================================================
> "Thread-11 
> (ActiveMQ-server-org.apache.activemq.artemis.core.server.impl.ActiveMQServerImpl$6@640f11a1)":
>   at 
> org.apache.activemq.artemis.core.protocol.mqtt.MQTTConnectionManager.disconnect(MQTTConnectionManager.java:150)
>   - waiting to lock <0x000000045f29f240> (a 
> org.apache.activemq.artemis.core.protocol.mqtt.MQTTSessionState)
>   at 
> org.apache.activemq.artemis.core.protocol.mqtt.MQTTFailureListener.connectionFailed(MQTTFailureListener.java:37)
>   at 
> org.apache.activemq.artemis.core.protocol.mqtt.MQTTConnection.fail(MQTTConnection.java:150)
>   at 
> org.apache.activemq.artemis.core.remoting.server.impl.RemotingServiceImpl$FailureCheckAndFlushThread$2.run(RemotingServiceImpl.java:780)
>   at 
> org.apache.activemq.artemis.utils.actors.OrderedExecutor.doTask(OrderedExecutor.java:42)
>   at 
> org.apache.activemq.artemis.utils.actors.OrderedExecutor.doTask(OrderedExecutor.java:31)
>   at 
> org.apache.activemq.artemis.utils.actors.ProcessorBase.executePendingTasks(ProcessorBase.java:65)
>   at 
> org.apache.activemq.artemis.utils.actors.ProcessorBase$$Lambda$137/0x0000000800e01dc8.run(Unknown
>  Source)
>   at 
> java.util.concurrent.ThreadPoolExecutor.runWorker(java.base@17.0.1/ThreadPoolExecutor.java:1136)
>   at 
> java.util.concurrent.ThreadPoolExecutor$Worker.run(java.base@17.0.1/ThreadPoolExecutor.java:635)
>   at 
> org.apache.activemq.artemis.utils.ActiveMQThreadFactory$1.run(ActiveMQThreadFactory.java:118)
> "Thread-24 
> (ActiveMQ-server-org.apache.activemq.artemis.core.server.impl.ActiveMQServerImpl$6@640f11a1)":
>   at 
> org.apache.activemq.artemis.core.protocol.mqtt.MQTTSession.start(MQTTSession.java:87)
>   - waiting to lock <0x000000045f2a1068> (a 
> org.apache.activemq.artemis.core.protocol.mqtt.MQTTSession)
>   at 
> org.apache.activemq.artemis.core.protocol.mqtt.MQTTConnectionManager.connect(MQTTConnectionManager.java:111)
>   - locked <0x000000045f29f240> (a 
> org.apache.activemq.artemis.core.protocol.mqtt.MQTTSessionState)
>   at 
> org.apache.activemq.artemis.core.protocol.mqtt.MQTTProtocolHandler.handleConnect(MQTTProtocolHandler.java:185)
>   at 
> org.apache.activemq.artemis.core.protocol.mqtt.MQTTProtocolHandler.act(MQTTProtocolHandler.java:133)
>   at 
> org.apache.activemq.artemis.core.protocol.mqtt.MQTTProtocolHandler$$Lambda$382/0x0000000801034000.onMessage(Unknown
>  Source)
>   at org.apache.activemq.artemis.utils.actors.Actor.doTask(Actor.java:33)
>   at 
> org.apache.activemq.artemis.utils.actors.ProcessorBase.executePendingTasks(ProcessorBase.java:65)
>   at 
> org.apache.activemq.artemis.utils.actors.ProcessorBase$$Lambda$137/0x0000000800e01dc8.run(Unknown
>  Source)
>   at 
> java.util.concurrent.ThreadPoolExecutor.runWorker(java.base@17.0.1/ThreadPoolExecutor.java:1136)
>   at 
> java.util.concurrent.ThreadPoolExecutor$Worker.run(java.base@17.0.1/ThreadPoolExecutor.java:635)
>   at 
> org.apache.activemq.artemis.utils.ActiveMQThreadFactory$1.run(ActiveMQThreadFactory.java:118)
> "Thread-88 
> (ActiveMQ-remoting-threads-ActiveMQServerImpl::name=0.0.0.0-212232499)":
>   at 
> org.apache.activemq.artemis.core.protocol.mqtt.MQTTSessionState.clear(MQTTSessionState.java:59)
>   - waiting to lock <0x000000045f29f240> (a 
> org.apache.activemq.artemis.core.protocol.mqtt.MQTTSessionState)
>   at 
> org.apache.activemq.artemis.core.protocol.mqtt.MQTTSession.clean(MQTTSession.java:200)
>   at 
> org.apache.activemq.artemis.core.protocol.mqtt.MQTTSession.stop(MQTTSession.java:115)
>   - locked <0x000000045f2a1068> (a 
> org.apache.activemq.artemis.core.protocol.mqtt.MQTTSession)
>   at 
> org.apache.activemq.artemis.core.protocol.mqtt.MQTTConnectionManager.disconnect(MQTTConnectionManager.java:155)
>   - locked <0x00000002223bc2d0> (a 
> org.apache.activemq.artemis.core.protocol.mqtt.MQTTSessionState)
>   at 
> org.apache.activemq.artemis.core.protocol.mqtt.MQTTFailureListener.connectionFailed(MQTTFailureListener.java:37)
>   at 
> org.apache.activemq.artemis.core.protocol.mqtt.MQTTConnection.fail(MQTTConnection.java:150)
>   at 
> org.apache.activemq.artemis.core.remoting.server.impl.RemotingServiceImpl.issueFailure(RemotingServiceImpl.java:606)
>   at 
> org.apache.activemq.artemis.core.remoting.server.impl.RemotingServiceImpl.connectionDestroyed(RemotingServiceImpl.java:587)
>   at 
> org.apache.activemq.artemis.core.remoting.impl.netty.NettyAcceptor$Listener.connectionDestroyed(NettyAcceptor.java:951)
>   at 
> org.apache.activemq.artemis.core.remoting.impl.netty.ActiveMQChannelHandler.lambda$channelInactive$0(ActiveMQChannelHandler.java:89)
>   at 
> org.apache.activemq.artemis.core.remoting.impl.netty.ActiveMQChannelHandler$$Lambda$419/0x0000000801103b48.run(Unknown
>  Source)
>   at 
> org.apache.activemq.artemis.utils.actors.OrderedExecutor.doTask(OrderedExecutor.java:42)
>   at 
> org.apache.activemq.artemis.utils.actors.OrderedExecutor.doTask(OrderedExecutor.java:31)
>   at 
> org.apache.activemq.artemis.utils.actors.ProcessorBase.executePendingTasks(ProcessorBase.java:65)
>   at 
> org.apache.activemq.artemis.utils.actors.ProcessorBase$$Lambda$137/0x0000000800e01dc8.run(Unknown
>  Source)
>   at 
> java.util.concurrent.ThreadPoolExecutor.runWorker(java.base@17.0.1/ThreadPoolExecutor.java:1136)
>   at 
> java.util.concurrent.ThreadPoolExecutor$Worker.run(java.base@17.0.1/ThreadPoolExecutor.java:635)
>   at 
> org.apache.activemq.artemis.utils.ActiveMQThreadFactory$1.run(ActiveMQThreadFactory.java:118)
> Found 1 deadlock.{noformat}
>  



--
This message was sent by Atlassian Jira
(v8.20.10#820010)

Reply via email to