Should it be possible to call `close` more than once?

[StarpTech]

Discussed first in https://github.com/mcollina/avvio/pull/56

[mcollina]

I'm 👎 . It should error on the second time (in the callback, reject in the promise). I probably added that part in a haste. That whole logic could be simplified.

[StarpTech]

I'm 👎 too

We should remove the queue mechanism in close and provide a single simple callback.

[StarpTech]

@mcollina in the current state a second close handler can't receive an err correct?

'use strict'

const avvio = require('.')()

avvio
  .ready(function () {
    console.log('application booted!')
    avvio.onClose((context, done) => {
      done(new Error('test'))
    })
    avvio.close((err, cb) => {
      console.log(err, '1111') // error, '1111'
      cb(err)
    })
    avvio.close((err) => {
      console.log(err, '2222') // null '2222'
    })
  })

We could release it as bugfix when we adjust the code as @delvedor https://github.com/mcollina/avvio/pull/56#issuecomment-377683090 suggested because the err must be handled by the user since the start. WDYT?

[StarpTech]

As the next step, we could remove the queue completely and this would be a major change.

[StarpTech]

Do you working on this or can I create a PR?

[mcollina]

go ahead and create a PR! Il giorno gio 5 apr 2018 alle 22:19 Dustin Deus [email protected] ha scritto:

Do you working on this or can I create a PR?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/mcollina/avvio/issues/57#issuecomment-379063721, or mute the thread https://github.com/notifications/unsubscribe-auth/AADL46CCeD-GLQ7YlKlAXgk04-mqH1l5ks5tlnxUgaJpZM4TCa_7 .

[StarpTech]

My plan would be:

Round 1

• Delvedor suggestion
• It's minor change because the error of close(err) could never be true but is present in the parameters.

Round 2

• Remove call of ready() inside close()
• Don't append close handler to closeQ queue support only a single callback per app.
• Major change

[delvedor]

Remove call of ready() inside close()

Why remove the ready call? If you have already started the app nothing will change, if the app is not yet started why call close in the first place?

Don't append close handler to closeQ queue support only a single callback per app.

Maybe I'm missing something here. What do you mean?

---

In my opinion call close more than once must return an error and should not be allowed.

[StarpTech]

Why remove the ready call?

For me, it's wrong that close handle more than it should. If you want to bootstrap the app call start or enable autostart period.

Maybe I'm missing something here. What do you mean?

It should only allow setting a single callback like

avvio.close(() => {}) // OK
avvio.close(() => {}) // error: close was already called

so, in that case, we don't need a queue.

[delvedor]

For me, it's wrong that close handle more than it should. If you want to bootstrap the app call start or enable autostart period.

I strongly suggest you to moderate your tone. While I can agree that close should just call the close queue, I think that is safer call ready, because call close will run all the onClose functions, and it can happen that one of them needs that a plugin has been started before to use them.

so, in that case, we don't need a queue.

There is not difference between the close queue and the onClose queue, we put the close callback at the end of the onClose queue just to notify the user when the closing process has finished.

[StarpTech]

I strongly suggest you to moderate your tone.

Don't be so pedantic! This is a technical discussion and I just make my point clear.

While I can agree that close should just call the close queue, I think that is safer call ready, because call close will run all the onClose functions, and it can happen that one of them needs that a plugin has been started before to use them.

Then you will mix up again two different things starting and closing. As a user, I expect that close will close my application and not start the plugin queue. When the plugin queue wasn't started then there is nothing to care about and when yes it should be fine that close() can only be called after ready().

[delvedor]

What's your problem?

Again.

Then you will mix up again two different things starting and closing. As a user, I expect that close will close my application and not start the plugin queue. When the plugin queue wasn't started then there is nothing to care about.

It is not a matter of mixing up, in theory a user should not call close before calling ready, it is useless and we all agree on this. The thing is that if a user calls close in a bad moment (during the application boot for example) we cannot run the close queue while the others plugins are still loading. @mcollina made this very clear in https://github.com/mcollina/avvio/issues/59#issuecomment-378565932. That ready is just to avoid this case, if you call close in the wrong moment, it will start at the right time.

[StarpTech]

It is not a matter of mixing up, in theory a user should not call close before calling ready, it is useless and we all agree on this.

• [X] Agree and this can be avoided by a check in close()

The thing is that if a user calls close in a bad moment (during the application boot for example) we cannot run the close queue while the others plugins are still loading. @mcollina made this very clear in #59

• [X] Yes, I understand this case but this can be avoided when we agree that close must be called after ready any other case is unpredictable and this change sounds reasonable. We could throw an error when the user call close before the queue drains.

[StarpTech]

• Throw error when a user try to call close before ready (or before plugin queue drains)
• Document that close can only be called after ready
• Don't call ready() inside close
• Throw error when close is called twice
• Don't append close handler to onClose queue and provide only a single callback handler

WDYT?

[mcollina]

This should work:

1. if we are booting, then wait for the boot to finish (call .ready()).
2. if we are not loading at all, do not call ready(), but rather make future ready()  calls error if we are already closing.
3. error (via callback or reject) if close is called twice.

We need to call .ready() while we are booting to support always calling .close() in tests.