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

Ignore and retry Connect() for non-registered clients with an option #281

Closed

Conversation

Praveenrajmani
Copy link

Hi ,

I have been playing around paho quite a time. Appreciate the work! I need few things to add upon to work some corner cases.

Events will not persist if the client is not connected/registered at least once.

Steps to reproduce

  • Dont start the mqtt broker yet
  • Send the events via Publish ( Wi th client configured with store )

We cannot see the events persisting in this case. The events are simply lost as the connection is not yet registered.

For repro, Try the following script

package main

import (
	"github.com/paho.mqtt.golang"
	"fmt"
	"time"
)


func main() {
	options := mqtt.NewClientOptions().
		SetClientID("123ccc").
		SetCleanSession(false). 
		AddBroker("tcp://localhost:1883").
		SetStore(mqtt.NewFileStore("/tmp/mqtt"))

	client := mqtt.NewClient(options)

	token := client.Connect()

	go func() {
		// Repeat the pings until the client registers the token.
		for {
			if token.Wait() && token.Error() == nil {
				fmt.Println("MQTT: Connected")
				break
			}
			fmt.Println("MQTT: " + token.Error().Error() + ". Retrying in 5 seconds.")
			time.Sleep(5 * time.Second)
			token = client.Connect()
		}
	}()

	go func() {
		for {
			// Message will not be persisted.
			token = client.Publish("minio123", 2, false, "hello")
			if token.Error() != nil {
				fmt.Println("message not sent. Token error: ", token.Error().Error())
			} else {
				fmt.Println("message sent")
				break
			}
			time.Sleep(2* time.Second)
		}
	}()

	for {}
}

@Praveenrajmani
Copy link
Author

Have to add some more changes/simplifications to this PR. Will update the it soon.

@balamurugana
Copy link

Please fix PR title and first comment specifically and also check ip-validation

@Praveenrajmani Praveenrajmani changed the title Add PublishNonRegisteredEvent for non-registered publish Ignore and retry Connect() for non-registered clients with an option Feb 19, 2019
Retry the connect() call until the client registers its token

Signed-off-by: Praveenrajmani <praveen@minio.io>
@MattBrittan
Copy link
Contributor

This looked like exactly what I needed - thanks!
Unfortunately taking a look at the code I think there are some issues (note that I don't know this code that well so may be wrong!):

  • For messageIds the code assumes that the max id will be len(c.persist.All())+1. The message ids are allocated at the time publish is called and then get sent out (or not) - this means that the lowest message id is quite likely to be above 0 if the client had been running previously (so the length of the store is not really related to the message ids in use). The only way I can see to deal with this is to iterate through the messages and get the ids.
  • The store is being opened in the goroutine so calling connect and then publish immediately may fail (because the goroutine may not have run yet). I also think that opening/closing the store on each iteration could cause issues (due to other goroutines using the store).
  • Once connect() is called there is no way to cancel this (it will only exit when the connection is successful or the application terminates).

I think these would all be fairly minor fixes. However as this request has not moved for some time and, if I'm reading things correctly, you no longer need it I'm guessing that it's not moving forward. As I needed the functionality I've implemented in a different way and will raise another pull request (but am happy for either to go forward - will be nice to have the ability to just call connect then publish without needing to worry if the message will get through).

@alsm
Copy link
Contributor

alsm commented Aug 29, 2019

Thanks for offering this and the idea, I'm closing as I believe the function is covered by the PR @MattBrittan made

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 this pull request may close these issues.

None yet

4 participants