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

Added StartInForeground to eventloop #77

Merged
merged 2 commits into from
Apr 18, 2024
Merged

Conversation

Cosrnos
Copy link
Contributor

@Cosrnos Cosrnos commented Apr 11, 2024

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
Copy link
Owner

dop251 commented Apr 17, 2024

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
Copy link
Contributor Author

Cosrnos commented Apr 17, 2024

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

@Cosrnos Cosrnos changed the title Added StartLooping to eventloop Added StartInForeground to eventloop Apr 18, 2024
@dop251 dop251 merged commit 2aae10d into dop251:master Apr 18, 2024
8 checks passed
@Cosrnos Cosrnos deleted the start-looping branch April 18, 2024 16:03
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

Successfully merging this pull request may close these issues.

2 participants