Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Problem of sending outgoingStore data even though CONNACK sessionPresent is false #1651

Open
ogis-yamazaki opened this issue Jul 27, 2023 · 6 comments
Labels

Comments

@ogis-yamazaki
Copy link
Contributor

If SessionState is not present, the broker returns false to sessionPresent in CONNACK.

In this case, I don't think the client should send an in-flight message (PUBLISH, PUBREL)
Since the session is gone, retransmissions of messages that occurred in the old session should not be sent.

I think we need to distinguish between the following two. I think it is the first one that should be deleted.

  1. a PUBLISH message generated in a previous session.
  2. a PUBLISH message created before the current session was created (between CONNECT and the return of CONNACK).

It would be nice to have a clear function in the Store.

(remarks)
The MQTT V5.0 specification has the following description.
OASIS : Session Expiry Interval https://docs.oasis-open.org/mqtt/mqtt/v5.0/os/mqtt-v5.0-os.html#_Toc3901048

Non-normative comment

The Client can avoid implementing its own Session expiry and instead rely on the Session Present flag returned from the Server to determine if the Session had expired. If the Client does implement its own Session expiry, it needs to store the time at which the Session State will be deleted as part of its Session State.
@robertsLando
Copy link
Member

In this case, I don't think the client should send an in-flight message (PUBLISH, PUBREL). Since the session is gone, retransmissions of messages that occurred in the old session should not be sent.

I'm not sure about this. If you consider a client with a clean session that is sending lot of messages and then suddenly the connection goes down and needs to reconnect, If there were messages QoS > 1 in outgoing store I expect them to be sent on reconnection

@ogis-yamazaki
Copy link
Contributor Author

@robertsLando

Sorry for the late reply.

The following definitions have been added in V5.0.
If sessionPresent is false, it seems to need to be destroyed.
At the very least, I think V5.0 needs to address this

[MQTT V5.0] 3.2.2.1.1 Session Present
https://docs.oasis-open.org/mqtt/mqtt/v5.0/os/mqtt-v5.0-os.html#_Toc3901078

If the Client does have Session State and receives Session Present set to 0 it MUST discard its Session State if it continues with the Network Connection

@robertsLando
Copy link
Member

@ogis-yamazaki Go for a PR and I will give it a check

@ogis-yamazaki
Copy link
Contributor Author

@robertsLando

I think the Store needs to implement a clear function that deletes all data from previous sessions.

https://github.com/mqttjs/MQTT.js/blob/main/src/lib/store.ts

In that case, the compatibility of the following libraries written in the README will be lost

Other implementations of mqtt.Store:

[mqtt-jsonl-store](https://github.com/robertsLando/mqtt-jsonl-store) which uses [jsonl-db](https://github.com/AlCalzone/jsonl-db) to store inflight data, it works only on Node.
[mqtt-level-store](http://npm.im/mqtt-level-store) which uses [Level-browserify](http://npm.im/level-browserify) to store the inflight data, making it usable both in Node and the Browser.
[mqtt-nedb-store](https://github.com/behrad/mqtt-nedb-store) which uses [nedb](https://www.npmjs.com/package/nedb) to store the inflight data.
[mqtt-localforage-store](http://npm.im/mqtt-localforage-store) which uses [localForage](http://npm.im/localforage) to store the inflight data, making it usable in the Browser without browserify.

Is there another good solution?

For now, I will create a PR with clear function.

@robertsLando
Copy link
Member

We could in the meanwhile use createStream and then del functions in packages that doesn't have its support. Could you do that in a PR?

Copy link

This is an automated message to let you know that this issue has
gone 365 days without any activity. In order to ensure that we work
on issues that still matter, this issue will be closed in 14 days.

If this issue is still important, you can simply comment with a
"bump" to keep it open.

Thank you for your contribution.

@github-actions github-actions bot added the stale label Oct 31, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants