[question] how do we want to handle async persistence setup?

[seriousme]

@robertsLando

I'm working on migration to the async interface of aedes persistence and I noticed that the broker is being set in the constructor of the class.

This means that we can never 'await' setup. We need to sneak in an await in here:

const aedes = require('aedes')()
const server = require('net').createServer(aedes.handle)
const port = 1883

server.listen(port, function () {
  console.log('server started and listening on port ', port)
})

so that we can await setup.

With the current callback interface this is solved by checking every call to persistence to see if ready which imo is also a bit flaky because if setup takes too long for some reason there will be quite some buffered calls to persistence and errors in setup might popup as other errors later.

So how do we deal with this? Some ideas:

1. explicit setup step

const aedes = require('aedes')()
await aedes.setup()
const server = require('net').createServer(aedes.handle)
const port = 1883

server.listen(port, function () {
  console.log('server started and listening on port ', port)
})

2. explicit call to persistence setup step

const aedes = require('aedes')()
await aedes.persistence.setup(aedes.broker)
const server = require('net').createServer(aedes.handle)
const port = 1883

server.listen(port, function () {
  console.log('server started and listening on port ', port)
})

3. implicit step

const aedes = await (require('aedes')())
const server = require('net').createServer(aedes.handle)
const port = 1883

server.listen(port, function () {
  console.log('server started and listening on port ', port)
})

4. invisible step

const aedes = require('aedes')()
const server = require('net').createServer(aedes.handle)
const port = 1883

server.listen(port, function () {
  console.log('server started and listening on port ', port)
})

In this case we call setup at the start of aedes.handle and await it there.

What do you think?

Kind regards, Hans

[robertsLando]

@seriousme it's not clear to me what you are referring to, do you mean https://github.com/moscajs/aedes/blob/main/aedes.js#L63 ?

[seriousme]

@robertsLando My bad, I should have explained it better!

The original aedes-persistence was synchronous. One would get an instance as soon as one called memory() https://github.com/moscajs/aedes/blob/main/aedes.js#L63

This works great for memory based persistence and although leveldb is strictly not synchronous, the startup of persistence-level was not a challenge. Enter Mongodb and Redis. Those really need to do some async work like connecting the client before persistence is ready to run.

aedes-cached-persistence used the setting of the broker (persistence.broker = .. to trigger setup and this emits ready when done and upon the ready event, persistence.ready was set.

We mimick this behaviour in https://github.com/seriousme/aedes-persistence/blob/9528bd4eb0b9009d67e1818e3c808079234a823c/callBackPersistence.js#L19-L36

Since this method does not await the setup of the persistence from the perspective of Aedes (it just sets persistence.broker) the creators of aedes-cached-persistence used a trick to avoid being too early with calls to persistence. We do the same in the callback interface to maintain compatibility,so every callback style call to a persistence method is guarded by:

  if (!this.ready) {
        this.once('ready', this.<method>.bind(this, <param>, cb))
        return this
      }

This works but only if setup does not hang etc, because else Aedes will happily add once events until some event handler limit is reached and the whole thing explodes with an error that is dificult to traceback to setup hanging, which imo is suboptimal.

Therefore the new Async interface makes a clear distinction between contstructor (no async stuff) and setup() which is async and can be awaited e.g.:

This can be very lightweight, as in the case of memory persistence: https://github.com/moscajs/aedes-persistence/blob/9528bd4eb0b9009d67e1818e3c808079234a823c/asyncPersistence.js#L83

Or quite some juggling with indexes etc for mongdb: https://github.com/moscajs/aedes-persistence-mongodb/blob/4552c54600668bf110a08050ec08da4a8beed845/asyncPersistence.js#L76

Now the easy trick of course is to call setup() from the Aedes constructor without awaiting a result (using async/await in a constructor is not allowed when using classes, not sure how that works with the pre-es6 prototype setup), however if setup for some reason takes a little more time then we might be to early with calls to persistence.

The proper solution is to await setup(broker) , but now the question is where do we put the await? Hence the 4 options.

An explicit await before we start handling requests (option 1,2,3) is more clean imo than trying to fix it on the first call to .handle() as this might introduce race conditions again (but users would not have to change their code in option 4)

Hope this explains it a bit better. Feel free to ask more questions!

Kind regards, Hans

[robertsLando]

@seriousme Ok thanks for the explaination!

IMO the problem doesn't exists because when the user wants to use a custom persistence it will pass the persistence instance in aedes options and we must explain that he must await persistence.setup() before creating the aedes instance:

const myPersistence = new PersisteceX()

await myPersistence.setup()

[... pass myPersistence to aedes options ...  ]

In case of memory persistence the problem doesn't exists as the setup is sync

[seriousme]

well actually, the broker needs to be passed to setup and only aedes knows about the broker (currently) If we want to do the explicit setup that leaves us 2 options:

Option 2

const aedes = require('aedes')()
await aedes.setup()

or

Option3

const aedes = require('aedes')()
await persistence.setup(aedes.broker)

where option 2 hides more of the implementation details. I think I prefer option 2, are you ok with this as well?

Kind regards, Hans