Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

'Close' should shutdown application even when there are plugins being processed #59

Closed
StarpTech opened this issue Apr 3, 2018 · 4 comments

Comments

@StarpTech
Copy link
Member

The close implementation should be simplified.

  • close should not start the queue.
  • close should shutdown the application anyway in its current state
'use strict'

const avvio = require('.')()

avvio.onClose(() => console.log('onClose!'))
avvio
  .use(first, { hello: 'world' })
  .ready(function () {
    console.log('application booted!')
  })

function first (instance, opts, cb) {
  console.log('first loaded', opts)
  instance.close(() => console.log('closed!'))
  // cb()
}

/**
first loaded { hello: 'world' }
*/

Proposal

A plugin should be able to close the application with close() regardless in which phase the application is in. This is very important to ensure a graceful shutdown.

When close() is called only the close queue should proceed and an error will pass when the ready queue readyQ.idle() returns false.

'use strict'

const avvio = require('.')()

avvio.onClose(() => console.log('onClose!'))
avvio
  .use(first, { hello: 'world' })
  .ready(function () {
    console.log('application booted!')
  })

function first (instance, opts, cb) {
  console.log('first loaded', opts)
  instance.close((err) => {
   console.log(err.message)
   console.log('closed!')
  })
  // cb()
}

/**
first loaded { hello: 'world' }
onClose!
Plugin queue is still in bootstrapping!
closed!
*/
@mcollina
Copy link
Member

mcollina commented Apr 4, 2018

close is implemented in the current way because of my lazyness and the fact that it was implemented in a rush. Ideally it should work in a similar way to what you describe (but not exactly).
It needs to wait for the current plugin to finish loading, and then process the close queue.

Adapted proposal:

'use strict'

const avvio = require('.')()

avvio.onClose(() => console.log('onClose!'))
avvio
  .use(first, { hello: 'world' })
  .ready(function () {
    console.log('application booted!')
  })

function first (instance, opts, cb) {
  console.log('first loaded', opts)
  instance.close((err) => {
   console.log(err.message)
   console.log('closed!')
  })
  cb() // this is required
}

@StarpTech
Copy link
Member Author

I didn't understand the difference. Could you get more in detail?

It needs to wait for the current plugin to finish loading, and then process the close queue.

But what happens when a plugin called .close() but some plugins will never be finished? The ready queue should not block the close queue.

@mcollina
Copy link
Member

mcollina commented Apr 4, 2018

The callback of the plugin needs to be called, and all the plugins up to that point called. The whole concept of avvio is avoiding race conditions and provide a dependable boot/close lifecycle.
If you allow close to start while it’s still booting, the end result could cause memory leaks and finish in an anomaly situation as the order of the loading and closing plugin would be random and hard to reproduce. When close is executed, no plugin should be loading.

Anyway, why do you want to call close while loading a plugin?

@StarpTech
Copy link
Member Author

StarpTech commented Apr 4, 2018

Always good to talk about it. I think I misused the api. The error handling should always be done by the done callback.

I intend to close my application due to a critical error of the client driver. The done callback was not called because an error was emitted and I tried to close the app.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants