[CHANGED] MQTT s.clear() do not wait for JS responses when disconnecting the session #5575

levb · 2024-06-20T22:16:22Z

MQTT s.clear(): do not wait for JS responses when disconnecting the session

Related to #5471

Previously we were making jsa.NewRequest as it is needed when connecting a clean session. On disconnect, there is no reason to wait for the response (and tie up the MQTT read loop of the client).

This should specifically help situations when a client app with many MQTT connections and QOS subscriptions disconnects suddenly, causing a flood of JSAPI deleteConsumer requests.

Test: n/a, not sure how to instrument for it.

kozlovic

The test TestMQTTSessionNotDeletedOnDeleteConsumerError fails because it was expecting an error that is no longer reported due to this change.

kozlovic · 2024-06-20T22:52:27Z

server/mqtt.go

@@ -1000,7 +1000,7 @@ func (s *Server) mqttHandleClosedClient(c *client) {

 	// This needs to be done outside of any lock.
 	if doClean {
-		if err := sess.clear(); err != nil {
+		if err := sess.clear(true); err != nil {


This will be invoked for "clean" sessions, which means sessions which state should be discarded and not reused in any subsequent session. Could there be a situation where a create/close/create causes an issue since some assets may not have been completely removed from the JS server?

We don't do anything with the error other than logging it anyway, so the logic doesn't change. Also, if the session is re-created as clean, we'd clean out its state on CONNECT. If it is re-created as persistent... 0/5 we could wipe out and re-create the consumers if there was a session message; but if we failed to delete the session message on the "clean" DISCONNECT... well, it still would not change the existing logic, would it?

kozlovic · 2024-06-20T22:56:37Z

server/mqtt.go

@@ -3823,7 +3829,7 @@ CHECK:
 			// This Session lasts as long as the Network Connection. State data
 			// associated with this Session MUST NOT be reused in any subsequent
 			// Session.
-			if err := es.clear(); err != nil {
+			if err := es.clear(false); err != nil {


At least here we make sure that the delete is complete, which I think is the right thing to do.

Right. I understand why checking the success is important here, thus preserved as before.

…ing the session

server/mqtt.go

levb · 2024-06-21T00:00:11Z

@kozlovic Sorry, I misunderstood the failing test. I thought the error in the log was supposed to come from JetStream itself, where it is actually the very MQTT error I skipped that the test was expecting.

I need a little time to re-think how to do this, there can still be errors sending the request to delete the consumer, so the test is still valid.

levb · 2024-06-21T00:36:02Z

@kozlovic here's my thinking.

0/5 for this PR we can keep the disconnect cleanup as opportunistic, and remove the failing TestMQTTSessionNotDeletedOnDeleteConsumerError. There really aren't any plausible errors in the "no wait" case, we just push a message onto the jsapi queue. So we will always attempt to delete the session message, just like before this PR.

Longer term:

For clean sessions, can we make subscription consumers ephemeral, with a short inactive threshold, so that they get auto-collected if we fail to delete?
For all new sessions, even clean, try to load the existing session message first. If the previous persisted session was clean, then synchronously delete its subs' consumers, we know the names. Ditto the pubrel consumer.
PubRel consumers can be consolidated, like the retained messages into one, account-specific consumer. There’s no need to match the messages to subs, just to the sessions, by client ID.

What do you think?

…nDeleteConsumerError

kozlovic

LGTM

kozlovic · 2024-06-21T17:13:38Z

Longer term:
For clean sessions, can we make subscription consumers ephemeral, with a short inactive threshold, so that they get auto-collected if we fail to delete?

But is not an ephemeral consumer supposed to be told that the "client" side is still active through pings? Meaning that would that mean that the MQTT code would then need to be more involved in maintaining this consumer? And if so, it means that if there was a disconnect between where the MQTT sub lives and the JS consumer leader, we could end-up with the consumer being deleted while the MQTT code does not know that this is the case and end-up with a stop in message delivery?

levb · 2024-06-21T18:14:00Z

@derekcollison This can be merged now.

…ing the session (#5575) MQTT s.clear(): do not wait for JS responses when disconnecting the session Related to #5471 Previously we were making `jsa.NewRequest` as it is needed when connecting a clean session. On disconnect, there is no reason to wait for the response (and tie up the MQTT read loop of the client). This should specifically help situations when a client app with many MQTT connections and QOS subscriptions disconnects suddenly, causing a flood of JSAPI deleteConsumer requests. Test: n/a, not sure how to instrument for it.

Includes: * #5571 * #5082 * #5573 * #5574 * #5575 * #5577 * #5578 * #5580 * #5559 Signed-off-by: Neil Twigg <neil@nats.io>

Built on top of #5575. Tries to prevent the stream `msgs` queue from becoming overwhelmed by core NATS publishes. In this case, where a reply subject is known, the sender will receive a 429 "Too Many Requests". Otherwise it's rate-logged. Two new configuration options are added to the JetStream block: `max_buffered_size` and `max_buffered_msgs`. If not configured, defaults are used. Signed-off-by: Neil Twigg <neil@nats.io> Signed-off-by: Neil Twigg <neil@nats.io>

levb requested review from derekcollison, neilalexander and kozlovic June 20, 2024 22:16

levb requested a review from a team as a code owner June 20, 2024 22:16

levb marked this pull request as draft June 20, 2024 22:39

kozlovic requested changes Jun 20, 2024

View reviewed changes

[CHANGED] MQTT s.clear() do not wait for JS responses when disconnect…

ddda72d

…ing the session

levb force-pushed the lev-leak-clean-nowait branch from 173649e to ddda72d Compare June 20, 2024 22:58

Removed commented out code

00efd84

levb commented Jun 20, 2024

View reviewed changes

server/mqtt.go Show resolved Hide resolved

levb commented Jun 20, 2024

View reviewed changes

server/mqtt.go Outdated Show resolved Hide resolved

levb commented Jun 20, 2024

View reviewed changes

server/mqtt.go Outdated Show resolved Hide resolved

levb requested a review from kozlovic June 20, 2024 23:15

levb marked this pull request as ready for review June 20, 2024 23:19

kozlovic requested changes Jun 20, 2024

View reviewed changes

server/mqtt.go Outdated Show resolved Hide resolved

server/mqtt.go Outdated Show resolved Hide resolved

fixes for moving nowait into NewRequest

25490e1

levb added 3 commits June 21, 2024 08:02

Reverted to the original approach, removed TestMQTTSessionNotDeletedO…

8c9faa7

…nDeleteConsumerError

removed dead code

539b79f

restored the original newRequest, not Ex

59db092

kozlovic approved these changes Jun 21, 2024

View reviewed changes

derekcollison merged commit ee9af0f into main Jun 21, 2024
4 checks passed

derekcollison deleted the lev-leak-clean-nowait branch June 21, 2024 19:11

neilalexander mentioned this pull request Jun 21, 2024

Cherry-picks for 2.10.17-RC.7 #5572

Merged

wallyqs added a commit that referenced this pull request Jun 21, 2024

Cherry-picks for 2.10.17-RC.7 (#5572)

8ef49c4

Includes: * #5571 * #5082 * #5573 * #5574 * #5575 * #5577 * #5578 * #5580 * #5559 Signed-off-by: Neil Twigg <neil@nats.io>

neilalexander mentioned this pull request Aug 16, 2024

Stream ingest rate limiting #5796

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[CHANGED] MQTT s.clear() do not wait for JS responses when disconnecting the session #5575

[CHANGED] MQTT s.clear() do not wait for JS responses when disconnecting the session #5575

levb commented Jun 20, 2024

kozlovic left a comment

kozlovic Jun 20, 2024

levb Jun 20, 2024

kozlovic Jun 20, 2024

levb Jun 20, 2024

levb commented Jun 21, 2024 •

edited

Loading

levb commented Jun 21, 2024

kozlovic left a comment

kozlovic commented Jun 21, 2024

levb commented Jun 21, 2024

[CHANGED] MQTT s.clear() do not wait for JS responses when disconnecting the session #5575

[CHANGED] MQTT s.clear() do not wait for JS responses when disconnecting the session #5575

Conversation

levb commented Jun 20, 2024

kozlovic left a comment

Choose a reason for hiding this comment

kozlovic Jun 20, 2024

Choose a reason for hiding this comment

levb Jun 20, 2024

Choose a reason for hiding this comment

kozlovic Jun 20, 2024

Choose a reason for hiding this comment

levb Jun 20, 2024

Choose a reason for hiding this comment

levb commented Jun 21, 2024 • edited Loading

levb commented Jun 21, 2024

kozlovic left a comment

Choose a reason for hiding this comment

kozlovic commented Jun 21, 2024

levb commented Jun 21, 2024

levb commented Jun 21, 2024 •

edited

Loading