openjs-foundation / summit

Organizing a Collaborator Summit
144 stars 58 forks source link

Session Proposal: NodeJS Asynchronous Scheduling #392

Closed ronag closed 7 months ago

ronag commented 9 months ago

Proposal

Topic of the session

Existing pitfalls in NodeJS asynchronous scheduling and patterns.

Type of the session

Estimated duration of the session

Date and Time of the session

Level

Pre-requisite knowledge

Describe the session

Session facilitator(s), Github handle(s) and timezone(s)

Meeting notes and Virtual Meeting Link

Follow-up / Set-up sessions (if any)

Additional context (optional)


ronag commented 9 months ago

Refs: https://github.com/nodejs/node/pull/51471 Refs: https://github.com/nodejs/node/pull/51114 Refs: https://github.com/nodejs/node/pull/51070 Refs: https://github.com/nodejs/node/issues/51156 Refs: https://github.com/nodejs/node/pull/51280 Refs: https://github.com/nodejs/undici/pull/2497

ronag commented 9 months ago

@mcollina

benjamingr commented 9 months ago

Would love to attend this remotely or discuss offline

ronag commented 9 months ago

Might be also relevant to make sure that the docs are correct:

https://www.builder.io/blog/visual-guide-to-nodejs-event-loop https://nodejs.org/en/learn/asynchronous-work/understanding-processnexttick

ronag commented 9 months ago

Example of problem to discuss.

import { EventEmitter } from 'events'

// Unhandled exception
setImmediate(async () => {
  const e = await new Promise(resolve => {
    const e = new EventEmitter()
    resolve(e)
    process.nextTick(() => {
      e.emit('error', new Error('nextTick'))
    })
  })
  e.on('error', () => {})
})

// Unhandled exception
setImmediate(async () => {
  const e = await new Promise(resolve => {
    const e = new EventEmitter()
    resolve(e)
    queueMicrotask(() => {
      e.emit('error', new Error('queueMicrotask'))
    })
  })
  e.on('error', () => {})
})

// OK, but slow
setImmediate(async () => {
  const e = await new Promise(resolve => {
    const e = new EventEmitter()
    resolve(e)
    setImmediate(() => {
      e.emit('error', new Error('setImmediate'))
    })
  })
  e.on('error', () => {})
})

Please don't get hung up the 'error' event. This is a problem with other events as well. In many cases they can be worse, e.g. a similar example with waiting for a 'ready' event would deadlock instead of crash.

benjamingr commented 9 months ago

I'm wondering if it's possible to solve generically by adding the capability to emit an event after someone is listening to it to the abstraction itself - e.g. by providing a helper to defer emitting an event up to setImmediate if there are no existing listeners for it.

This is similar to what we do for promises at the moment with unhanadled rejection detection.

littledan commented 7 months ago

Can we toss into this topic, how are web platform APIs for managing asynchronicity like AbortController working for your needs in Node? Maybe that is too much of a tangent and can stick in the standards part, though.

joyeecheung commented 7 months ago

For the remote participants: we've scheduled Zoom Webinars for the sessions, please register using the links provided in https://github.com/openjs-foundation/summit/issues/387 and you'll get a link in an email to join the sessions. More info in the issue mentioned.

EDIT: You can also get invited to be a panelist beforehand to save the registration & promotion step. Ping in the OpenJS slack with your email or send a email to the email in my GitHub profile to get an invitation.

Qard commented 7 months ago

A possible solution is to allow EventEmitter to take a buffer strategy which can optionally buffer events to be flushed later. For example:

const events = require('events')

class EventEmitter extends events.EventEmitter {
  constructor(options) {
    super(options)
    this.buffer = options?.bufferStrategy || new NoBufferStrategy()
  }

  emit(name, ...args) {
    // No listeners yet. Try to buffer the event.
    if (!this.listeners(name).length && this.buffer.buffer(name, args)) {
      return
    }

    return super.emit(name, ...args)
  }

  on(name, ...args) {
    try {
      return super.on(name, ...args)
    } finally {
      // After attaching a handler, flush any buffered events.
      this.buffer.flush(name, (...args) => {
        this.emit(...args)
      })
    }
  }
}

// Default strategy does no buffering.
class NoBufferStrategy {
  buffer(name, task) {
    return false
  }

  flush() {}
}

// A possible alternative strategy could buffer the events until a handler is given.
class BufferStrategy {
  #buffered = new Map()

  buffer(name, args) {
    const map = this.#buffered

    if (!map.has(name)) {
      map.set(name, [])
    }
    map.get(name).push(args)

    return true
  }

  flush(name, handle) {
    const map = this.#buffered
    const queue = map.get(name)
    map.delete(name)

    for (const args of queue) {
      handle(name, ...args)
    }
  }
}

const p = new Promise((resolve) => {
  const e = new EventEmitter({
    bufferStrategy: new BufferStrategy()
  })

  process.nextTick(() => {
    e.emit('error', 'nextTick')
  })

  resolve(e)
})

p.then(e => {
  return new Promise(resolve => {
    setImmediate(resolve, e)
  })
}).then(e => {
  e.on('error', (error) => console.log('error', error))
})

It's worth noting that the BufferStrategy here is just very simplistic example to demonstrate the idea. It probably should not buffer indefinitely, it should have limits. At the least it should probably ensure error events don't just get held forever when there is no handler.

joyeecheung commented 7 months ago

Closing as the session has ended.

Links to the recordings:

Links to the notes (very inaccurate, we should try harder at getting dedicated note takers the next time):

Drafted trip report, would appreciate some reviews. Expect to publish on the official blog next week: https://hackmd.io/5vvP8o5bTmqcbNh-3oHpag