mcollina / mqtt-level-store

Store your in-flight MQTT message on Level, for Node
MIT License
24 stars 11 forks source link

Added insertion order support. #11

Closed redboltz closed 6 years ago

redboltz commented 6 years ago

Added cmd type check on put. If cmd type is the same before and after update, update is aboted.

In the MQTT communication, update is only required from 'publish' to 'pubrel'.

redboltz commented 6 years ago

I fixed all code that you pointed out. Could you check it?

Some of 'should resend messages by published order' on travis-ci reported 2000ms timeout. It doesn't happen on my local environment. Could you restart failed test. Or should I do something?

redboltz commented 6 years ago

I tested the PR with debug print on my travis-ci environment.

https://travis-ci.org/redboltz/mqtt-level-store/jobs/420888797#L715

  beforeEach(function (done) {
    console.log('new server')
    server = new net.Server()
    console.log('server listen')
    server.listen(8883, done)

    console.log('server before connect')
    server.on('connection', function (stream) {
      console.log('server after connect')
      var client = Connection(stream)

      client.on('connect', function () {
        console.log('client on connect')
        client.connack({returnCode: 0})
      })

      console.log('emit client')
      server.emit('client', client)
    })

    console.log('create level store')
    manager = mqttLevelStore({ level: level() })
  })

  afterEach(function (done) {
    console.log('after each')
    manager.close(function () {
      console.log('manager closed')
      server.close(function () {  // *** block here ***
        console.log('server closed')
        done()
      })
    })
  })

I noticed that server.close callback sometimes not called on travis-ci. Maybe there is a better way. I started read MQTT.js's test code. If you know something about that, please give me your advise.

redboltz commented 6 years ago

During try and error, I commented out the test 'should resend messages' https://github.com/mcollina/mqtt-level-store/blob/master/test.js#L64 , the test 'should resend messages by published order' passed. It indicates that there is some problem to do multiple tests in the test suite 'mqtt.connect flow'.

I think that we have two choice.

  1. Remove 'should resend messages'. The test is to check resend one message. 'should resend messages by published order' is to check resend three messages. I think that the latter covers the former. It is the easiest way for me.

  2. Introduce a proper way to do multiple tests in the test suite 'mqtt.connect flow' on travis-ci. I think that is the best way. But I don't know how to do it. The current master seems to support only one test.

redboltz commented 6 years ago

Here is a diff of approach 1 branch and master:

diff

And travis-ci result(stable success): https://travis-ci.org/redboltz/mqtt-level-store/builds/420910228

mcollina commented 6 years ago

CI is a bit flaky, can you improve the stability of the tests? Also, can you add Node 10?

redboltz commented 6 years ago

Do you mean I can use approach 1? It’s very stable. And I will add node 10.

mcollina commented 6 years ago

Yeah, you can do 1.

redboltz commented 6 years ago

I bumped a compile error when compiling leveldown on nodejs 10.

I googled the error and got many error reports.

I couldn't find a way to solve it. But it is not directly related to mqtt-level-store. So I added nodejs 9 instead of 10. I think that it is acceptable for now.

mcollina commented 6 years ago

That's perfect, thanks!