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

MQTT-support #766

Open
wants to merge 9 commits into
base: main
Choose a base branch
from
Open

MQTT-support #766

wants to merge 9 commits into from

Conversation

kickster97
Copy link
Member

@kickster97 kickster97 commented Aug 26, 2024

WHAT is this pull request doing?

An MQTT-session inherits AMQP::Queue and extends with MQTT functionality.
MQTT exchange(topic) will only accept activity from an MQTT client, that will be responsible for routing messages to the MQTT-session.
The mqtt exchange handles MQTT routing keys etc. without needing to do conversions.

NOTE
The first version of MQTT Support does not support binding an AMQP exchange to and MQTT exchange. (Undefined behaviour if done)
In a separate PR we need to extract the AMQP exchange types into the AMQP namespace and in turn be able to differentiate between an AMQP::Destination and an MQTT::Destiantion when calling bind.
With that in place we will be able to safely do exchange to exchange bindings.

Docs for the website

HOW can this pull request be tested?

Specs have been migrated from Myra and fully cover the MQTT protocol, run with crystal spec.
Connect with mqtt client library of your choice and test your usage against this branch

static/js/connections.js Outdated Show resolved Hide resolved
static/js/connections.js Outdated Show resolved Hide resolved
src/lavinmq/vhost.cr Outdated Show resolved Hide resolved
src/lavinmq/vhost.cr Outdated Show resolved Hide resolved
src/lavinmq/mqtt/client.cr Outdated Show resolved Hide resolved
src/lavinmq/mqtt/client.cr Outdated Show resolved Hide resolved
src/lavinmq/mqtt/client.cr Outdated Show resolved Hide resolved
src/lavinmq/mqtt/broker.cr Outdated Show resolved Hide resolved
@spuun
Copy link
Member

spuun commented Sep 24, 2024

I think you can (and maybe even should) build clean session by reusing the auto delete feature of queues.

@spuun
Copy link
Member

spuun commented Oct 12, 2024

About #803 (comment)

That connect spec passes, but for the wrong reason. Or well, the socket is closed but not because it receives the wrong packet type, but because of the bug that #803 will fix. When that bug is fixed i still think that the spec will pass, but for the right reason.

I guess the spec could be changed to send a PUBLISH instead (the packet must be greater than 8 byte to not trigger the bug).

@spuun
Copy link
Member

spuun commented Oct 12, 2024

After giving it a second thought I've concluded that we should change the spec to use some other packet type.

@kickster97
Copy link
Member Author

After giving it a second thought I've concluded that we should change the spec to use some other packet type.

ok I will change the spec then! thanks :)

@carlhoerberg
Copy link
Member

Often it's more efficient to yield than to capture and then call a block.

src/lavinmq/mqtt/broker.cr Outdated Show resolved Hide resolved
src/lavinmq/exchange/mqtt.cr Outdated Show resolved Hide resolved
src/lavinmq/mqtt/client.cr Outdated Show resolved Hide resolved
src/lavinmq/mqtt/client.cr Outdated Show resolved Hide resolved
src/lavinmq/mqtt/client.cr Outdated Show resolved Hide resolved
src/lavinmq/mqtt/client.cr Outdated Show resolved Hide resolved
src/lavinmq/vhost.cr Outdated Show resolved Hide resolved
src/lavinmq/mqtt/session.cr Outdated Show resolved Hide resolved
src/lavinmq/mqtt/session.cr Outdated Show resolved Hide resolved
src/lavinmq/mqtt/session.cr Outdated Show resolved Hide resolved
src/lavinmq/amqp/client.cr Outdated Show resolved Hide resolved
src/lavinmq/amqp/exchange/exchange.cr Outdated Show resolved Hide resolved
src/lavinmq/amqp/exchange/exchange.cr Outdated Show resolved Hide resolved
src/lavinmq/config.cr Outdated Show resolved Hide resolved
src/lavinmq/mqtt/broker.cr Outdated Show resolved Hide resolved
src/lavinmq/queue_factory.cr Outdated Show resolved Hide resolved
src/lavinmq/server.cr Outdated Show resolved Hide resolved
src/lavinmq/server.cr Outdated Show resolved Hide resolved
src/lavinmq/vhost_store.cr Show resolved Hide resolved
static/js/connections.js Outdated Show resolved Hide resolved
Copy link
Member

@carlhoerberg carlhoerberg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

only halfway through

CHANGELOG.md Outdated Show resolved Hide resolved
spec/mqtt/keepalive_spec.cr Show resolved Hide resolved
Comment on lines +144 to +165
# This macro will find the "bind" method of classes inheriting from this class
# and redefine them to raise AccessRefused exception if the first argument
# isn't a type in LavinMQ::AMQP namespace.
#
# TODO remove this when LavinMQ::MQTT::Session no longer inherit from
# LavinMQ::AMQP::Queue and LavinMQ::MQTT::Exchange no longer inherit from
# lavinMQ::AMQP::Exchange
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I call this black magic!

But the TODO note was a relief

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this can be done explicitly instead? can't be that many methods?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The next step after merging would be to separate Session from Queue, so this is very temporary

src/lavinmq/config.cr Outdated Show resolved Hide resolved
src/lavinmq/mqtt/broker.cr Outdated Show resolved Hide resolved
src/lavinmq/mqtt/client.cr Outdated Show resolved Hide resolved
src/lavinmq/mqtt/connection_factory.cr Outdated Show resolved Hide resolved
src/lavinmq/mqtt/exchange.cr Outdated Show resolved Hide resolved
src/lavinmq/mqtt/exchange.cr Outdated Show resolved Hide resolved
src/lavinmq/mqtt/exchange.cr Outdated Show resolved Hide resolved
@carlhoerberg
Copy link
Member

please rebase, and squash as many commits as possible.

src/lavinmq/mqtt/session.cr Outdated Show resolved Hide resolved
src/lavinmq/mqtt/session.cr Show resolved Hide resolved
src/lavinmq/mqtt/session.cr Show resolved Hide resolved
src/lavinmq/mqtt/session.cr Outdated Show resolved Hide resolved
src/lavinmq/mqtt/session.cr Outdated Show resolved Hide resolved
src/lavinmq/mqtt/subscription_tree.cr Outdated Show resolved Hide resolved
src/lavinmq/mqtt/subscription_tree.cr Outdated Show resolved Hide resolved
src/lavinmq/mqtt/subscription_tree.cr Outdated Show resolved Hide resolved
@kickster97 kickster97 force-pushed the mqtt-poc branch 2 times, most recently from 76917ad to bd762b2 Compare January 28, 2025 13:05
Copy link
Member

@spuun spuun left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It start to come together!

spec/message_routing_spec.cr Outdated Show resolved Hide resolved
spec/mqtt/integrations/message_qos_spec.cr Outdated Show resolved Hide resolved
Copy link
Member

@snichme snichme left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have reviewed the code, ran the specs locally. I know there has been testing done locally with real mqtt clients. It looks good, so lets get this merged! :)

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.

4 participants