gausby / tortoise

A MQTT Client written in Elixir
Apache License 2.0
312 stars 54 forks source link

Not defining a handler prevents messages from being published #117

Closed Dipricyn closed 4 years ago

Dipricyn commented 4 years ago

Consider the following example:

defmodule Test2.Application do
  use Application

  @impl true
  def start(_type, _args) do
    Tortoise.Supervisor.start_child(
      client_id: "local",
      # not setting a handler causes not being able to publish
      # handler: {Tortoise.Handler.Logger, []},
      server: {Tortoise.Transport.Tcp, host: 'localhost', port: 1883},
      subscriptions: []
    )

    Tortoise.publish("local", "test_topic", "test_payload")

    children = []
    Supervisor.start_link(children, strategy: :one_for_one)
  end
end

Notice that the line where the handler is defined is commented out.

As I didn't want the Tortoise.Handler.Logger printing connection information to stdout, I thought removing the handler would not print any output.

As it turns out this also prevents any messages from being published.

Wouldn't it be better to use Tortoise.Handler.Default as the default if the option is not defined?

gausby commented 4 years ago

Not sure if I am following this…no message is published to the MQTT server if you omit the handler in the config? Do you have any error messages in the stdout when you omit it?

Can't you just use the Tortoise.Handler.Default handler if you don't want to see log messages printed to stdout?

Dipricyn commented 4 years ago

Not sure if I am following this…no message is published to the MQTT server if you omit the handler in the config?

Right. Probably because the application crashes and is restarted afterwards.

Do you have any error messages in the stdout when you omit it?

I didn't get any error messages that's why the problem was particularly hard to identify.

Can't you just use the Tortoise.Handler.Default handler if you don't want to see log messages printed to stdout?

Yes, that works. I just thought that it was counterintuitive to manually have to specify a Tortoise.Handler in order to effectively use no handler. I'd recommend setting the Tortoise.Handler.Default handler if the user doesn't specify any handler.

gausby commented 4 years ago

I think I will let the upcoming version of Tortoise crash if no handler has been specified. The handler will be more important in the future, as MQTT5 is more complex, and the user will have to take more callbacks into consideration.

Sorry for the late reply. I have been busy with life, and I hope I can get to add more maintainers when I am done with the MQTT 5 branch. Hope tortoise works well for you—would it be okay for me to close this issue ?

Dipricyn commented 4 years ago

I think I will let the upcoming version of Tortoise crash if no handler has been specified. The handler will be more important in the future, as MQTT5 is more complex, and the user will have to take more callbacks into consideration.

If it is necessary with MQTT 5 to always set a handler, then that's ok. The point I am trying to make is that if the library can be used without a handler, then the user shouldn't manually have to specify that they want to use the Tortoise.Handler.Default handler.

If MQTT 5 requires the user to always define a custom handler, then this issue probably won't appear.

gausby commented 4 years ago

I like that the user need to make a conscious choice of what should happen in the MQTT connection life-cycle. As such the Logger handler is only included to make it easy to get going, and for people to have an example to work from when they start implementing their own handler—the default one is there to use in integration tests where I don't care about the log output.

Dipricyn commented 4 years ago

I see. I get your design decision. The issue can be closed.

gausby commented 4 years ago

Cool; once again, thanks for using Tortoise…I am always happy to hear what you are using it for, so please share if you want and are allowed to :)