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

Persistence is not implemented #25

Closed
MattBrittan opened this issue Jul 22, 2020 · 6 comments · Fixed by #172
Closed

Persistence is not implemented #25

MattBrittan opened this issue Jul 22, 2020 · 6 comments · Fixed by #172

Comments

@MattBrittan
Copy link
Contributor

Describe the bug
The package includes a Persistence interface along with two implementations (noopPersistence and MemoryPersistence) but these are not currently used. I have classed this as a bug rather than a feature request because it's partially implemented (and therefore likely to be confusing to new users).

To reproduce
Attempt to use the Persistence implementations.

Debug output
N/A

Expected behaviour
Messages persist as per the MQTT spec.

Software used:

  • Server was - N/A
  • Client version 7369e71

Additional context
Mentioned in #23

alsm pushed a commit to alsm/paho.golang that referenced this issue Jul 23, 2020
I've removed the OnDisconnect callback and moved to OnConnectionLost
instead and used a specific Error type to indicate when it was a server
initiated disconnect (see test change)

eclipse-paho#25
eclipse-paho#26
@MattBrittan
Copy link
Contributor Author

I am taking another look at the implementation of persistence. A few initial thoughts:

  1. Initially only outgoing persistence will be implemented (on the basis that the PR enabling user control of ACK's will be implemented so it will be possible to effectively use the broker to handle inbound persistence). This will mean that the package will not fully comply with the spec (and QOS 2 will guarantee no duplication) but keeps things simple (it will be possible to add this later).
  2. Persistence handling will be completely user definable. My initial thought is that all inbound and outbound comms will be added to a channel (rather than being handled/transmitted directly) and the routine that processes these messages could be replaced (which would provide maximum flexibility, including the functionality requested in How about adding hook for PUBXXX messages #40).

Anyway thought I should add a note to indicate that there are plans to do this (no guarantee on a time-frame sorry) ; if anyone has any thoughts/requirements then please let me know so I can feed these into the design.

@yanmxa
Copy link

yanmxa commented Aug 1, 2023

@MattBrittan Do you have a plan for how long to enable the persistence or cleansession=false in this repos?

We've been trying to use this session feature recently with this library, and I've also integrated this MQTT protocol into CloudEvents, maybe more people will pay attention to this later.

@MattBrittan
Copy link
Contributor Author

@yanmxa I'm actually working on it right now. However I don't know if my current attempt will be successful (or, if it is, how long it will take). The challenge is implementing this in a way that keeps the library simple and idiomatic (really want to avoid the complexity of the v3.1 client).

@MattBrittan
Copy link
Contributor Author

After three years of minimal activity I'm thrilled to say that we now have two potential solutions to this issue (links to repos below):

These solutions are quite different approaches and will each have advantages/disadvantages. I think it's fair to say that the solution from @vishnureddy17 is incremental (same basic structure but does significantly change network subroutines) whereas mine is a more fundamental change (decouples the session from paho which may, or may not, be a good idea!).

Both of these repos should be treated as a work in progress; they both contain working code but there are likely to be bugs (and my code definitely needs tidying up!).

We are really keen to receive feedback, along with any other ideas on how best to approach this. Whichever approach we end up taking this is going to be a major change (and I'm sure some existing code will break), but it's important that we get this right (and it's not easy - most libraries do not fully support persistent session state).

Anyway I'm really happy that the road to V1 is now open again (this issue needs to be closed before that because of the likelihood that there will be breaking changes).

@ashtonian
Copy link

is there an equivalent clientconfig.SetConnectionLostHandler() ?

@MattBrittan
Copy link
Contributor Author

SetConnectionLostHandler

Your comment does not seem directly related to this issue? Paho has OnClientError which will be called if there is a network error.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants