moscajs / mosca

MQTT broker as a module
mosca.io
3.2k stars 509 forks source link

Retained message delivery fails with Mosca, yet works with Mosquitto (websockets build) #328

Closed cefn closed 9 years ago

cefn commented 9 years ago

I have the following test case in which Mosca does not deliver the retained message as expected. However, Mosquitto built with Websockets support running on port 3000 does deliver the retained message as per spec.

https://gist.github.com/cefn/2e87416aa24bfdddbb7d

To switch between Mosca and Mosquitto, shutdown the Mosquitto process and substitute the promiseMosquitto() line with promiseMosca().

You can get Mosca to successfully deliver the message by moving the 'then' clause containing promiseSubscription to the start of the chain, making sure that the receiver client's topic subscription completes before the sender client's topic publication.

This suggests that perhaps retained messages are not working at all on the version of Mosca I downloaded from npm.

I have minimised the library dependencies to just Mosca and MQTT, based on

npm install --save mosca mqtt

...but it relies heavily on the inbuilt Promise implementations in NodeJS 12 which is hard to avoid.

I am using the following Mosquitto 1.4.2 configuration for testing on Ubuntu...

autosave_interval 30
persistence true
persistence_file mosquitto.db
connection_messages true
log_timestamp true

listener 3000 127.0.0.1
protocol websockets

To build Mosquitto with websockets support I changed config.mk to WITH_WEBSOCKETS=yes and WITH_SRV=no after installing libwebsockets-dev and uuid-dev and following http://www.xappsoftware.com/wordpress/2015/05/18/six-steps-to-install-mosquitto-1-4-2-with-websockets-on-debian-wheezy/.

mcollina commented 9 years ago

Again, MQTT.js does not support Promises out of the box. Please do it without promises, I fear the problem might be there.

cefn commented 9 years ago

It's totally fair to expect me to rewrite without Promises if that's what you prefer as a bug report. No problem there.

However, Promises are just a syntactic sugar on callbacks, so the idea that MQTT.js needs to support promises is a little confusing to me. I'm not making any invocations which expects MQTT.js to support promises, however, I am using promises to sequence callbacks. I'll try it with a 'christmas tree' style instead.

Using promises simplifies the presentation of this test case, and in particular makes it easier to switch the order of completion of subscription and publication without somehow embedding this in closures or callbacks. I'll see how comprehensible I can make the test case with a simple callback convention instead.

mcollina commented 9 years ago

This is surely wrong: https://gist.github.com/cefn/2e87416aa24bfdddbb7d#file-retain_fail-js-L36-L40 It leaks a function inside the EventEmitter.

cefn commented 9 years ago

Hi again. I think you mean that each time the promiseReceipt() call is made, it creates and subscribes a listener to the EventEmitter. You're totally right you wouldn't do such a thing in a real implementation just to get a promise for the next message. I think it's not related to the failing test though.

However, as a test case in which promiseReceipt() is called only once in an example sequence of connecting, listening subscribing and publishing, it seemed harmless, and minimises the code in the test while exposing the test logic (the chain is executed once and the error is immediately detectable).

This was deliberately constructed as a minimal test to demonstrate the problem I'm experiencing, but I'm sure there could be a subtle bug I've put in there which is hidden by the use of Promises, so I totally appreciate a callback-based test is needed. Sorry not to have put together a callback equivalent, yet. Not had much dev time today.

In practice because MQTT message listener subscription is monolothic (not per-topic) I tend to subscribe a single message handler once then look after topic routing with my own stream-based logic using filters. These strategies rely on other libraries which would just confuse things if I included them.

For example in the live implementation, I've been using Kefir streams (which assume EventEmitter subscription and unsubscription e.g. https://rpominov.github.io/kefir/#from-event or https://rpominov.github.io/kefir/#stream ) and I derive Promise logic from those if I ever need to trigger a callback for a future event (e.g. https://rpominov.github.io/kefir/#to-promise ). Both Kefir and Bacon FRP libraries execute a single subscription whenever an operation starts consuming the stream and unsubscription when they stop consuming, so the management of EventEmitter listeners is automagic.

Did I understand your comment correctly? You may have meant something else. Sorry if I missed the point.

mcollina commented 9 years ago

I cannot reproduce using the CLI tools, retained messages works as expected everywhere.

Also, it seems you are not configuring a persistence in your code: https://gist.github.com/cefn/2e87416aa24bfdddbb7d#file-retain_fail-js-L5-L12. A persistence is needed to store retained messages.

Try adding a persistence.

cefn commented 8 years ago

Thanks @mcollina adding persistence (when I worked out how to do it correctly) fixed the issue.

I had a previous persistence configuration but it was incorrect, and I had removed it to make a more minimal test case, not realising it was needed for retained messages.

The functioning configuration is shown at https://gist.github.com/cefn/2e87416aa24bfdddbb7d#gistcomment-1628193