dop251 / goja_nodejs

Nodejs compatibility library for Goja
MIT License
329 stars 78 forks source link

Added StartInForeground to eventloop #77

Closed Cosrnos closed 5 months ago

Cosrnos commented 5 months ago

Problem

The application I'm working on right now has some logic that will recover from panics at the highest level to perform some additional logging (mostly of internal state that we'll need to use later as part of recovering from unplanned downtime).

For our scripting interface, we're generally able to manage panics that arise from go calls in our scripts if they happen within a synchronous call (i.e. RunOnLoop & RunProgram calls) but if the problematic code executes in a background event loop during a setTimeout we're currently unable to perform our shutdown tasks.

So for example main.js

setTimeout(function() {
   context.goFunctionThatPanics(); // Panics in a goroutine and crashes the program without logging
})();

We do have a workaround at the moment by just spawning a goroutine to call Run with an empty function

go func() {
  defer func() {
      if rec := recover(); rec != nil {
          // Call Panic Recovery
      }
  }()

  for {
      loop.Run(func(vm *goja.Runtime) {
          // Tick
      })
  }
}() 

However this feels a bit hacky

Proposed Solution

Simply provide an interface for calling the loop directly in the current goroutine

func (loop *EventLoop) StartLooping() {
    loop.setRunning()
    loop.run(true)
}

Admittedly, this might be somewhat of a niche problem, or it might be that our current solution is the intended interface but I figured I'd open a PR as a starting point for discussion.

Let me know if you have any thoughts or would prefer another route for solving this problem -- I'm happy to contribute any way I can!

dop251 commented 5 months ago

Hi. Thanks for submitting this. It looks good to me, except the name, maybe call it StartInForeground() or something to make it more obvious? Also I think it's worth mentioning the use case (i.e. recovering from panics) in the documentation.

Cosrnos commented 5 months ago

@dop251 Great! I've updated my branch with those changes. Let me know if you need anything else!