[jira] [Work logged] (ARTEMIS-4646) Unacknowledged MQTT message ID is reused after ID generator wraparound
[ https://issues.apache.org/jira/browse/ARTEMIS-4646?focusedWorklogId=906495&page=com.atlassian.jira.plugin.system.issuetabpanels:worklog-tabpanel#worklog-906495 ] ASF GitHub Bot logged work on ARTEMIS-4646: --- Author: ASF GitHub Bot Created on: 22/Feb/24 16:55 Start Date: 22/Feb/24 16:55 Worklog Time Spent: 10m Work Description: jbertram commented on PR #4823: URL: https://github.com/apache/activemq-artemis/pull/4823#issuecomment-1959864883 @thezbyg, nice work. Thanks for the contribution! Issue Time Tracking --- Worklog Id: (was: 906495) Time Spent: 1h 20m (was: 1h 10m) > Unacknowledged MQTT message ID is reused after ID generator wraparound > -- > > Key: ARTEMIS-4646 > URL: https://issues.apache.org/jira/browse/ARTEMIS-4646 > Project: ActiveMQ Artemis > Issue Type: Bug > Components: MQTT >Reporter: Albertas Vyšniauskas >Priority: Major > Time Spent: 1h 20m > Remaining Estimate: 0h > > Hello, > MQTT message IDs are currently generated by incrementing *ids* variable in > *{color:#00}{color:#00}OutboundStore{color}{color}* class and do not > perform any additional checks before trusting that it is unique. This works > great until one or more MQTT messages are not acknowledged in timely manner > while receiving and acknowledging other messages. A different message with > the same ID will be sent after 32767 additional messages are received and > acknowledged. > There are also more issues with MQTT message ID generation: > # *AtomicInteger* is used for *ids* variable, but all accesses and > modifications are done in synchronized context, so there is no need for it to > be atomic. > # Maximum allowed *ids* value before wraparound is {*}Short.MAX_VALUE{*}, > but MQTT protocol allows values in the range of 1 to 65535 (all unsigned > short values except 0). > # Wraparound is done to value 1 and is immediately incremented thus skipping > message ID 1 and using 2 as first ID after wraparound. > # There is a harmless but wrong attempt to remove from *mqttToServerIds* > hash map by non-MQTT message ID in the following code: > \{_}mqttToServerIds.remove(p.getA());{_}. It is harmless because _p.getA()_ > returns *Long* value and will never match any key in *mqttToServerIds* hash > map which uses *Integer* keys.{*}{*} -- This message was sent by Atlassian Jira (v8.20.10#820010)
[jira] [Work logged] (ARTEMIS-4646) Unacknowledged MQTT message ID is reused after ID generator wraparound
[ https://issues.apache.org/jira/browse/ARTEMIS-4646?focusedWorklogId=906494&page=com.atlassian.jira.plugin.system.issuetabpanels:worklog-tabpanel#worklog-906494 ] ASF GitHub Bot logged work on ARTEMIS-4646: --- Author: ASF GitHub Bot Created on: 22/Feb/24 16:54 Start Date: 22/Feb/24 16:54 Worklog Time Spent: 10m Work Description: jbertram merged PR #4823: URL: https://github.com/apache/activemq-artemis/pull/4823 Issue Time Tracking --- Worklog Id: (was: 906494) Time Spent: 1h 10m (was: 1h) > Unacknowledged MQTT message ID is reused after ID generator wraparound > -- > > Key: ARTEMIS-4646 > URL: https://issues.apache.org/jira/browse/ARTEMIS-4646 > Project: ActiveMQ Artemis > Issue Type: Bug > Components: MQTT >Reporter: Albertas Vyšniauskas >Priority: Major > Time Spent: 1h 10m > Remaining Estimate: 0h > > Hello, > MQTT message IDs are currently generated by incrementing *ids* variable in > *{color:#00}{color:#00}OutboundStore{color}{color}* class and do not > perform any additional checks before trusting that it is unique. This works > great until one or more MQTT messages are not acknowledged in timely manner > while receiving and acknowledging other messages. A different message with > the same ID will be sent after 32767 additional messages are received and > acknowledged. > There are also more issues with MQTT message ID generation: > # *AtomicInteger* is used for *ids* variable, but all accesses and > modifications are done in synchronized context, so there is no need for it to > be atomic. > # Maximum allowed *ids* value before wraparound is {*}Short.MAX_VALUE{*}, > but MQTT protocol allows values in the range of 1 to 65535 (all unsigned > short values except 0). > # Wraparound is done to value 1 and is immediately incremented thus skipping > message ID 1 and using 2 as first ID after wraparound. > # There is a harmless but wrong attempt to remove from *mqttToServerIds* > hash map by non-MQTT message ID in the following code: > \{_}mqttToServerIds.remove(p.getA());{_}. It is harmless because _p.getA()_ > returns *Long* value and will never match any key in *mqttToServerIds* hash > map which uses *Integer* keys.{*}{*} -- This message was sent by Atlassian Jira (v8.20.10#820010)
[jira] [Work logged] (ARTEMIS-4646) Unacknowledged MQTT message ID is reused after ID generator wraparound
[ https://issues.apache.org/jira/browse/ARTEMIS-4646?focusedWorklogId=906093&page=com.atlassian.jira.plugin.system.issuetabpanels:worklog-tabpanel#worklog-906093 ] ASF GitHub Bot logged work on ARTEMIS-4646: --- Author: ASF GitHub Bot Created on: 20/Feb/24 21:06 Start Date: 20/Feb/24 21:06 Worklog Time Spent: 10m Work Description: thezbyg commented on code in PR #4823: URL: https://github.com/apache/activemq-artemis/pull/4823#discussion_r1496507334 ## tests/integration-tests/src/test/java/org/apache/activemq/artemis/tests/integration/mqtt/MQTTTest.java: ## @@ -447,6 +447,59 @@ public void testSendMoreThanUniqueId() throws Exception { publisher.disconnect(); } + @Ignore + @Test(timeout = 600 * 1000) + public void testNoMessageIdReuseBeforeAcknowledgment() throws Exception { Review Comment: Fixed test by using two initial messages. Previously it would not fail as first message had ID 1 and first ID after wraparound was 2. Issue Time Tracking --- Worklog Id: (was: 906093) Time Spent: 1h (was: 50m) > Unacknowledged MQTT message ID is reused after ID generator wraparound > -- > > Key: ARTEMIS-4646 > URL: https://issues.apache.org/jira/browse/ARTEMIS-4646 > Project: ActiveMQ Artemis > Issue Type: Bug > Components: MQTT >Reporter: Albertas Vyšniauskas >Priority: Major > Time Spent: 1h > Remaining Estimate: 0h > > Hello, > MQTT message IDs are currently generated by incrementing *ids* variable in > *{color:#00}{color:#00}OutboundStore{color}{color}* class and do not > perform any additional checks before trusting that it is unique. This works > great until one or more MQTT messages are not acknowledged in timely manner > while receiving and acknowledging other messages. A different message with > the same ID will be sent after 32767 additional messages are received and > acknowledged. > There are also more issues with MQTT message ID generation: > # *AtomicInteger* is used for *ids* variable, but all accesses and > modifications are done in synchronized context, so there is no need for it to > be atomic. > # Maximum allowed *ids* value before wraparound is {*}Short.MAX_VALUE{*}, > but MQTT protocol allows values in the range of 1 to 65535 (all unsigned > short values except 0). > # Wraparound is done to value 1 and is immediately incremented thus skipping > message ID 1 and using 2 as first ID after wraparound. > # There is a harmless but wrong attempt to remove from *mqttToServerIds* > hash map by non-MQTT message ID in the following code: > \{_}mqttToServerIds.remove(p.getA());{_}. It is harmless because _p.getA()_ > returns *Long* value and will never match any key in *mqttToServerIds* hash > map which uses *Integer* keys.{*}{*} -- This message was sent by Atlassian Jira (v8.20.10#820010)
[jira] [Work logged] (ARTEMIS-4646) Unacknowledged MQTT message ID is reused after ID generator wraparound
[ https://issues.apache.org/jira/browse/ARTEMIS-4646?focusedWorklogId=906092&page=com.atlassian.jira.plugin.system.issuetabpanels:worklog-tabpanel#worklog-906092 ] ASF GitHub Bot logged work on ARTEMIS-4646: --- Author: ASF GitHub Bot Created on: 20/Feb/24 21:02 Start Date: 20/Feb/24 21:02 Worklog Time Spent: 10m Work Description: thezbyg commented on code in PR #4823: URL: https://github.com/apache/activemq-artemis/pull/4823#discussion_r1496502532 ## tests/integration-tests/src/test/java/org/apache/activemq/artemis/tests/integration/mqtt/MQTTTest.java: ## @@ -447,6 +447,59 @@ public void testSendMoreThanUniqueId() throws Exception { publisher.disconnect(); } + @Ignore Review Comment: I noticed a similar ignored test **testSendMoreThanUniqueId** and assumed that this new test should also be ignored because it also takes a long time run. Issue Time Tracking --- Worklog Id: (was: 906092) Time Spent: 50m (was: 40m) > Unacknowledged MQTT message ID is reused after ID generator wraparound > -- > > Key: ARTEMIS-4646 > URL: https://issues.apache.org/jira/browse/ARTEMIS-4646 > Project: ActiveMQ Artemis > Issue Type: Bug > Components: MQTT >Reporter: Albertas Vyšniauskas >Priority: Major > Time Spent: 50m > Remaining Estimate: 0h > > Hello, > MQTT message IDs are currently generated by incrementing *ids* variable in > *{color:#00}{color:#00}OutboundStore{color}{color}* class and do not > perform any additional checks before trusting that it is unique. This works > great until one or more MQTT messages are not acknowledged in timely manner > while receiving and acknowledging other messages. A different message with > the same ID will be sent after 32767 additional messages are received and > acknowledged. > There are also more issues with MQTT message ID generation: > # *AtomicInteger* is used for *ids* variable, but all accesses and > modifications are done in synchronized context, so there is no need for it to > be atomic. > # Maximum allowed *ids* value before wraparound is {*}Short.MAX_VALUE{*}, > but MQTT protocol allows values in the range of 1 to 65535 (all unsigned > short values except 0). > # Wraparound is done to value 1 and is immediately incremented thus skipping > message ID 1 and using 2 as first ID after wraparound. > # There is a harmless but wrong attempt to remove from *mqttToServerIds* > hash map by non-MQTT message ID in the following code: > \{_}mqttToServerIds.remove(p.getA());{_}. It is harmless because _p.getA()_ > returns *Long* value and will never match any key in *mqttToServerIds* hash > map which uses *Integer* keys.{*}{*} -- This message was sent by Atlassian Jira (v8.20.10#820010)
[jira] [Work logged] (ARTEMIS-4646) Unacknowledged MQTT message ID is reused after ID generator wraparound
[ https://issues.apache.org/jira/browse/ARTEMIS-4646?focusedWorklogId=905781&page=com.atlassian.jira.plugin.system.issuetabpanels:worklog-tabpanel#worklog-905781 ] ASF GitHub Bot logged work on ARTEMIS-4646: --- Author: ASF GitHub Bot Created on: 19/Feb/24 22:41 Start Date: 19/Feb/24 22:41 Worklog Time Spent: 10m Work Description: jbertram commented on code in PR #4823: URL: https://github.com/apache/activemq-artemis/pull/4823#discussion_r1495065245 ## tests/integration-tests/src/test/java/org/apache/activemq/artemis/tests/integration/mqtt/MQTTTest.java: ## @@ -447,6 +447,59 @@ public void testSendMoreThanUniqueId() throws Exception { publisher.disconnect(); } + @Ignore + @Test(timeout = 600 * 1000) + public void testNoMessageIdReuseBeforeAcknowledgment() throws Exception { Review Comment: This test doesn't fail without your fix. In order to demonstrate the problem and mitigate regressions the test should fail without the fix and succeed with it. Issue Time Tracking --- Worklog Id: (was: 905781) Time Spent: 40m (was: 0.5h) > Unacknowledged MQTT message ID is reused after ID generator wraparound > -- > > Key: ARTEMIS-4646 > URL: https://issues.apache.org/jira/browse/ARTEMIS-4646 > Project: ActiveMQ Artemis > Issue Type: Bug > Components: MQTT >Reporter: Albertas Vyšniauskas >Priority: Major > Time Spent: 40m > Remaining Estimate: 0h > > Hello, > MQTT message IDs are currently generated by incrementing *ids* variable in > *{color:#00}{color:#00}OutboundStore{color}{color}* class and do not > perform any additional checks before trusting that it is unique. This works > great until one or more MQTT messages are not acknowledged in timely manner > while receiving and acknowledging other messages. A different message with > the same ID will be sent after 32767 additional messages are received and > acknowledged. > There are also more issues with MQTT message ID generation: > # *AtomicInteger* is used for *ids* variable, but all accesses and > modifications are done in synchronized context, so there is no need for it to > be atomic. > # Maximum allowed *ids* value before wraparound is {*}Short.MAX_VALUE{*}, > but MQTT protocol allows values in the range of 1 to 65535 (all unsigned > short values except 0). > # Wraparound is done to value 1 and is immediately incremented thus skipping > message ID 1 and using 2 as first ID after wraparound. > # There is a harmless but wrong attempt to remove from *mqttToServerIds* > hash map by non-MQTT message ID in the following code: > \{_}mqttToServerIds.remove(p.getA());{_}. It is harmless because _p.getA()_ > returns *Long* value and will never match any key in *mqttToServerIds* hash > map which uses *Integer* keys.{*}{*} -- This message was sent by Atlassian Jira (v8.20.10#820010)
[jira] [Work logged] (ARTEMIS-4646) Unacknowledged MQTT message ID is reused after ID generator wraparound
[ https://issues.apache.org/jira/browse/ARTEMIS-4646?focusedWorklogId=905780&page=com.atlassian.jira.plugin.system.issuetabpanels:worklog-tabpanel#worklog-905780 ] ASF GitHub Bot logged work on ARTEMIS-4646: --- Author: ASF GitHub Bot Created on: 19/Feb/24 22:40 Start Date: 19/Feb/24 22:40 Worklog Time Spent: 10m Work Description: jbertram commented on code in PR #4823: URL: https://github.com/apache/activemq-artemis/pull/4823#discussion_r1495064960 ## tests/integration-tests/src/test/java/org/apache/activemq/artemis/tests/integration/mqtt/MQTTTest.java: ## @@ -447,6 +447,59 @@ public void testSendMoreThanUniqueId() throws Exception { publisher.disconnect(); } + @Ignore Review Comment: How come this test is ignored? Issue Time Tracking --- Worklog Id: (was: 905780) Time Spent: 0.5h (was: 20m) > Unacknowledged MQTT message ID is reused after ID generator wraparound > -- > > Key: ARTEMIS-4646 > URL: https://issues.apache.org/jira/browse/ARTEMIS-4646 > Project: ActiveMQ Artemis > Issue Type: Bug > Components: MQTT >Reporter: Albertas Vyšniauskas >Priority: Major > Time Spent: 0.5h > Remaining Estimate: 0h > > Hello, > MQTT message IDs are currently generated by incrementing *ids* variable in > *{color:#00}{color:#00}OutboundStore{color}{color}* class and do not > perform any additional checks before trusting that it is unique. This works > great until one or more MQTT messages are not acknowledged in timely manner > while receiving and acknowledging other messages. A different message with > the same ID will be sent after 32767 additional messages are received and > acknowledged. > There are also more issues with MQTT message ID generation: > # *AtomicInteger* is used for *ids* variable, but all accesses and > modifications are done in synchronized context, so there is no need for it to > be atomic. > # Maximum allowed *ids* value before wraparound is {*}Short.MAX_VALUE{*}, > but MQTT protocol allows values in the range of 1 to 65535 (all unsigned > short values except 0). > # Wraparound is done to value 1 and is immediately incremented thus skipping > message ID 1 and using 2 as first ID after wraparound. > # There is a harmless but wrong attempt to remove from *mqttToServerIds* > hash map by non-MQTT message ID in the following code: > \{_}mqttToServerIds.remove(p.getA());{_}. It is harmless because _p.getA()_ > returns *Long* value and will never match any key in *mqttToServerIds* hash > map which uses *Integer* keys.{*}{*} -- This message was sent by Atlassian Jira (v8.20.10#820010)
[jira] [Work logged] (ARTEMIS-4646) Unacknowledged MQTT message ID is reused after ID generator wraparound
[ https://issues.apache.org/jira/browse/ARTEMIS-4646?focusedWorklogId=905779&page=com.atlassian.jira.plugin.system.issuetabpanels:worklog-tabpanel#worklog-905779 ] ASF GitHub Bot logged work on ARTEMIS-4646: --- Author: ASF GitHub Bot Created on: 19/Feb/24 22:40 Start Date: 19/Feb/24 22:40 Worklog Time Spent: 10m Work Description: jbertram commented on code in PR #4823: URL: https://github.com/apache/activemq-artemis/pull/4823#discussion_r1495064560 ## artemis-protocols/artemis-mqtt-protocol/src/main/java/org/apache/activemq/artemis/core/protocol/mqtt/MQTTSessionState.java: ## @@ -461,8 +459,14 @@ public int generateMqttId(long messageId, long consumerId) { synchronized (dataStoreLock) { Integer id = artemisToMqttMessageMap.get(generateKey(messageId, consumerId)); if (id == null) { - ids.compareAndSet(Short.MAX_VALUE, 1); - id = ids.addAndGet(1); + do { + if (currentId == 0x) { Review Comment: You can use `org.apache.activemq.artemis.core.protocol.mqtt.MQTTUtil#TWO_BYTE_INT_MAX` here instead of `0x`. Issue Time Tracking --- Worklog Id: (was: 905779) Time Spent: 20m (was: 10m) > Unacknowledged MQTT message ID is reused after ID generator wraparound > -- > > Key: ARTEMIS-4646 > URL: https://issues.apache.org/jira/browse/ARTEMIS-4646 > Project: ActiveMQ Artemis > Issue Type: Bug > Components: MQTT >Reporter: Albertas Vyšniauskas >Priority: Major > Time Spent: 20m > Remaining Estimate: 0h > > Hello, > MQTT message IDs are currently generated by incrementing *ids* variable in > *{color:#00}{color:#00}OutboundStore{color}{color}* class and do not > perform any additional checks before trusting that it is unique. This works > great until one or more MQTT messages are not acknowledged in timely manner > while receiving and acknowledging other messages. A different message with > the same ID will be sent after 32767 additional messages are received and > acknowledged. > There are also more issues with MQTT message ID generation: > # *AtomicInteger* is used for *ids* variable, but all accesses and > modifications are done in synchronized context, so there is no need for it to > be atomic. > # Maximum allowed *ids* value before wraparound is {*}Short.MAX_VALUE{*}, > but MQTT protocol allows values in the range of 1 to 65535 (all unsigned > short values except 0). > # Wraparound is done to value 1 and is immediately incremented thus skipping > message ID 1 and using 2 as first ID after wraparound. > # There is a harmless but wrong attempt to remove from *mqttToServerIds* > hash map by non-MQTT message ID in the following code: > \{_}mqttToServerIds.remove(p.getA());{_}. It is harmless because _p.getA()_ > returns *Long* value and will never match any key in *mqttToServerIds* hash > map which uses *Integer* keys.{*}{*} -- This message was sent by Atlassian Jira (v8.20.10#820010)
[jira] [Work logged] (ARTEMIS-4646) Unacknowledged MQTT message ID is reused after ID generator wraparound
[ https://issues.apache.org/jira/browse/ARTEMIS-4646?focusedWorklogId=905254&page=com.atlassian.jira.plugin.system.issuetabpanels:worklog-tabpanel#worklog-905254 ] ASF GitHub Bot logged work on ARTEMIS-4646: --- Author: ASF GitHub Bot Created on: 16/Feb/24 10:25 Start Date: 16/Feb/24 10:25 Worklog Time Spent: 10m Work Description: thezbyg opened a new pull request, #4823: URL: https://github.com/apache/activemq-artemis/pull/4823 Generate MQTT message IDs from full allowed range of 1-65535 and skip currently used values. Do not use atomic integer for current ID, because all accesses and modifications are performed in synchronized context. Issue Time Tracking --- Worklog Id: (was: 905254) Remaining Estimate: 0h Time Spent: 10m > Unacknowledged MQTT message ID is reused after ID generator wraparound > -- > > Key: ARTEMIS-4646 > URL: https://issues.apache.org/jira/browse/ARTEMIS-4646 > Project: ActiveMQ Artemis > Issue Type: Bug > Components: MQTT >Reporter: Albertas Vyšniauskas >Priority: Major > Time Spent: 10m > Remaining Estimate: 0h > > Hello, > MQTT message IDs are currently generated by incrementing *ids* variable in > *{color:#00}{color:#00}OutboundStore{color}{color}* class and do not > perform any additional checks before trusting that it is unique. This works > great until one or more MQTT messages are not acknowledged in timely manner > while receiving and acknowledging other messages. A different message with > the same ID will be sent after 32767 additional messages are received and > acknowledged. > There are also more issues with MQTT message ID generation: > # *AtomicInteger* is used for *ids* variable, but all accesses and > modifications are done in synchronized context, so there is no need for it to > be atomic. > # Maximum allowed *ids* value before wraparound is {*}Short.MAX_VALUE{*}, > but MQTT protocol allows values in the range of 1 to 65535 (all unsigned > short values except 0). > # Wraparound is done to value 1 and is immediately incremented thus skipping > message ID 1 and using 2 as first ID after wraparound. > # There is a harmless but wrong attempt to remove from *mqttToServerIds* > hash map by non-MQTT message ID in the following code: > \{_}mqttToServerIds.remove(p.getA());{_}. It is harmless because _p.getA()_ > returns *Long* value and will never match any key in *mqttToServerIds* hash > map which uses *Integer* keys.{*}{*} -- This message was sent by Atlassian Jira (v8.20.10#820010)