Generic MQTT improvements

[relu91]

Given the latest problems that MQTT gave, I tried to revamp it a little bit the implementation in the hope that it would cause fewer problems and would be more maintainable. I took three steps:

• Upgrade the library to the latest MQTT implementation -> we get async methods -> cleaner implementation and cover corner cases where we forgot to use callbacks.
• While I was there I updated the notation with the latest MQTT vocabulary
• I tried to tackle the issue raised in MQTT topic listener is not removed if the topic is unsubscribed #955 by @ilovemilk . Now it should be fixed but give the current design of the Scripting API (and node-wot). the implementation is not that clean. I refer to the fact that we can't keep track of each listener by using functions as handlers but we use forms as identifiers. The end result is that if in your thing description you have two forms from different events that share a mqv:filter you can't subscribe to two events at the same time. I hope this is a corner case... if anybody else has suggestions on this let me know!

Meanwhile I hope that this new changes also solves the problems in MacOS

[codecov]

Codecov Report

Attention: 60 lines in your changes are missing coverage. Please review.

Comparison is base (be1fd88) 76.66% compared to head (712ec51) 76.74%.

Files	Patch %	Lines
packages/binding-mqtt/src/mqtt-client.ts	56.98%	40 Missing ⚠️
packages/binding-mqtt/src/mqtt-message-pool.ts	88.00%	12 Missing ⚠️
packages/binding-mqtt/src/util.ts	77.77%	8 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #1184      +/-   ##
==========================================
+ Coverage   76.66%   76.74%   +0.07%     
==========================================
  Files          80       82       +2     
  Lines       16813    16946     +133     
  Branches     1618     1632      +14     
==========================================
+ Hits        12890    13005     +115     
- Misses       3893     3911      +18     
  Partials       30       30              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[relu91]

Meanwhile I hope that this new changes also solves the problems in MacOS

It seems it didn't let me try to play with it little more...

[relu91]

In b79e59b I tried to force qos 2 for every event. I suspect that the test is falling due to the fact that by default the event is published with qos 0 and therefore we lost some events. If it works I then try to improve the solution and make it more generic.

[relu91]

Ok now is failing due to file client (which fails also locally). @JKRhb can you fix in a parallel PR?

Nvm I must have blanked for a moment. It is still MQTT. (but locally windows the binding-file is falling I have to investigate my setup).

[relu91]

Ah maybe I did see it correctly:

[danielpeintner]

Nvm I must have blanked for a moment. It is still MQTT. (but locally windows the binding-file is falling I have to investigate my setup).

It is not just failing locally.. I have seen it failing also on the CI
https://github.com/eclipse-thingweb/node-wot/actions/runs/7112903861/job/19363790481#step:8:236

[relu91]

Note that while we get all green here. Locally, binding-file still fails:

File Client Implementation
    ✔ should be able to write and read files using URI scheme file:/// with Content-Type application/json
    1) should be able to write and read files using URI scheme file:/// with no Content-Type
    ✔ should be able to write and read files using URI scheme file:/// with Content-Type text/plain
    ✔ should be able to write and read files using URI scheme file:// with Content-Type application/json
    ✔ should be able to write and read files using URI scheme file:// with no Content-Type
    ✔ should be able to write and read files using URI scheme file:// with Content-Type text/plain


  5 passing (15ms)
  1 failing

  1) File Client Implementation
       should be able to write and read files using URI scheme file:/// with no Content-Type:
     AssertionError: expected undefined to deeply equal { foo: 'bar' }
      at  path\to\node-wot\binding-file\test\file-client-test.ts:90:43
      at Generator.next (<anonymous>)
      at fulfilled (test\file-client-test.ts:5:58)
      at processTicksAndRejections (node:internal/process/task_queues:95:5)

[JKRhb]

With #1185, the file-client related issues will hopefully be resolved soon :)

[danielpeintner]

@egekorkan do you think you can review this MQTT PR?
I am probably not the right person ... anyone else feel free to review/comment as well!

[relu91]

I rebased with the master to start on the changes that @JKRhb made. Let's wait for the final greenlight from the CI/CD and @egekorkan.

[relu91]

Ok CI/CD is still angry. The funny thing is that I have a Windows setup with Node 18.x and even if I run the tests sequentially like the following:

for i in {1..100}; do npm run test -w packages/binding-mqtt; done

I get 100% success rate. I think we need more information if we want to know more about why is falling. Could it be that it finds some occupied port and just waits there? Currently, we are firing 5 events and the code is waiting for 3... with qos 2 it is possible that we lost so many packages (they shouldn't be lost at all!).

Any idea how to debug better? Shall we activate DEBUG=node-wot:binding-mqtt:* on the CI/CD?

[JKRhb]

Ok CI/CD is still angry. The funny thing is that I have a Windows setup with Node 18.x and even if I run the tests sequentially like the following:

Hmm, that's very strange :/

Any idea how to debug better? Shall we activate DEBUG=node-wot:binding-mqtt:* on the CI/CD?

That sounds good! Maybe we can somehow configure the CI to re-run failed workflow runs with logging enabled in general? Could be useful to determine the source of problems more quickly. Edit: Just found an Action that could be used for that: https://github.com/marketplace/actions/retry-step#run-different-command-after-first-failure

[egekorkan]

Thanks a lot @relu91. The changes are many but all on the right direction!

[relu91]

That sounds good! Maybe we can somehow configure the CI to re-run failed workflow runs with logging enabled in general? Could be useful to determine the source of problems more quickly. Edit: Just found an Action that could be used for that: https://github.com/marketplace/actions/retry-step#run-different-command-after-first-failure

It could be handy yes, however, I'd probably need to enable debug for the whole node-wot package (i.e. DEBUG=node-wot). What's your feeling about it? does it work?

[JKRhb]

That sounds good! Maybe we can somehow configure the CI to re-run failed workflow runs with logging enabled in general? Could be useful to determine the source of problems more quickly. Edit: Just found an Action that could be used for that: https://github.com/marketplace/actions/retry-step#run-different-command-after-first-failure

It could be handy yes, however, I'd probably need to enable debug for the whole node-wot package (i.e. DEBUG=node-wot). What's your feeling about it? does it work?

I've just opened #1189 as an experimental PR to test it out :)

[danielpeintner]

Thanks a lot @relu91. The changes are many but all on the right direction!

@relu91 Once you feel comfortable I think you can merge!

[relu91]

Ok, I review the current status and I think we can merge it. There is still the problem of how properly generate forms in the mqtt broker following user preference (i.e. how the user can configure qos when exposing a Thing). We can improve it later and as it has also some implications with #1029. Also thanks to #1189 we can debug better what's happening.