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

broker [NET-573]: MQTT plugin improvements #272

Merged
merged 4 commits into from
Nov 9, 2021
Merged

Conversation

teogeb
Copy link
Contributor

@teogeb teogeb commented Nov 8, 2021

Fixes:

  1. A message was delivered to a subscriber twice if it was published by a MQTT client. Now we store message chain ids of the message which are published via MQTT server. We don't re-deliver those messages as Aedes mirrors the messages to all subscribers. (Aedes doesn't have a feature for disabling mirroring. If it had, that would be an alternative way to implement the functionality.)

  2. A message was delivered multiple times if there were multiple subscribers. Now we subscribe to streamrClient only once. Messages are delivered to the MQTT server and it propagates the messages to all subscribers.

  3. A subscription was unsubscribed if any of the clients unsubscribed the topic. Now we store list of clientIds and unsubscribes only if all clients have unsubscribed.

  4. Unsubscribes can't interfere other plugins. The MQTT plugin explictly unsubscribes from the subscriptions it has created.

@linear
Copy link

linear bot commented Nov 8, 2021

NET-573 Bug in mqtt broker plugin

There is a bug with the mqttplugin (not legacy) in Bridge.ts in the onSubscribed method:

each time subscribe is called on the mqttbroker, a new listener is added, that subscribes to the stream on streamrnetwork.

The bug can be reproduced on the tmp_bug_mqttplugin branch, where the legacymqtt plugin is replaced with the mqttplugin in the local-propagation integration test.

Run the test by running npm run t in broker package

Open questions:

  • mirroring: if you publish a message to a stream, and you are also subscribed to the stream how many times should it be mirrored back to you (currently without bug it would be 2 times: one time by the mqtt server inside the mqttplugin and one time by the streamr broker being subscribed to the same streamID on streams)

    User would probably expect it to be mirrored once

  • muliple mqtt clients being connected to one broker (and thus mqtt-plugin): if one unsubscribes from a stream, the other mqtt client will also stop getting messages from that stream.

    possible solution: keep track of mqttclients who are subscribed to a stream; get mqtt-clients id out of the subscribe(to mqqt-topic) event, or number them

  • if there are different plugins, like mqtt and ws, and you subscribe to a streamr stream over one of them, should the other plugin also get messages from that stream without explicitly subscribing itself? (probably not?)

@teogeb teogeb requested a review from samt1803 November 8, 2021 12:55
@github-actions github-actions bot added the broker Related to Broker Package label Nov 8, 2021
Copy link
Contributor

@samt1803 samt1803 left a comment

Choose a reason for hiding this comment

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

looks good!

@teogeb teogeb merged commit 5580072 into main Nov 9, 2021
@teogeb teogeb deleted the NET-573-mqtt-plugin branch November 9, 2021 11:51
samt1803 added a commit that referenced this pull request Nov 9, 2021
…rge_main

* origin/main: (34 commits)
  broker [NET-573]: MQTT plugin improvements (#272)
  style(client): fix eslint error
  Fix broken browser tests (#269)
  client [NET-571]: Using the Brubeck client without providing a private key (#268)
  ci: specify exact action versions for easy debugability (#266)
  ci(deps): bump actions/checkout from 2.3.5 to 2.4.0 (#271)
  docs(broker): fix mqtt example
  fix(broker): rely on default value for config.network.webrtcDisallowPrivateAddresses
  fix(broker): fix default value type in schema
  release: cli-tools 6.0.0-alpha.0
  fix: cli-tools work with alpha client
  test(network): fix describe
  Update README.md
  style(broker): break up log line
  ci(test-utils): replace styfle/cancel-workflow-action with GitHub Actions concurrency
  ci(protocol): replace styfle/cancel-workflow-action with GitHub Actions concurrency
  ci(network): replace styfle/cancel-workflow-action with GitHub Actions concurrency
  ci(client-dataunions): replace styfle/cancel-workflow-action with GitHub Actions concurrency
  ci(client-code): replace styfle/cancel-workflow-action with GitHub Actions concurrency
  ci(client-build): replace styfle/cancel-workflow-action with GitHub Actions concurrency
  ...

# Conflicts:
#	packages/broker/src/broker.ts
#	packages/cli-tools/src/publish.ts
timoxley added a commit that referenced this pull request Nov 9, 2021
* main:
  ci: Use nick-invision/[email protected] instead of nick-invision/[email protected].
  broker [NET-573]: MQTT plugin improvements (#272)
timoxley added a commit that referenced this pull request Nov 9, 2021
* NET-542-brubeck-client-browser:
  fix(broker): Fix merge issues.
  ci: Use nick-invision/[email protected] instead of nick-invision/[email protected].
  broker [NET-573]: MQTT plugin improvements (#272)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
broker Related to Broker Package
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants