Skip to content

This issue was moved to a discussion.

You can continue the conversation there. Go to discussion →

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

Single event loop #136

Closed
inancgumus opened this issue Nov 25, 2021 · 3 comments
Closed

Single event loop #136

inancgumus opened this issue Nov 25, 2021 · 3 comments
Labels
enhancement New feature or request events CDP or internal events related. idea 💡

Comments

@inancgumus
Copy link
Member

inancgumus commented Nov 25, 2021

I've been thinking about this today:

  • We might need a single event loop.
  • Instead of multiple event-loops scattered around the system.

So we can easily manage the chain of events in a single place and fix the problems faster and robustly. I believe, doing so will simplify the codebase and allow us to become more productive in time.

My reasoning is the current design makes debugging challenging and creates hard to grok synchronization issues. I know we follow Playwright code structure but we might want a different approach since we are using Go instead of Javascript. It's because each has its own style when architecting code.

You can't easily follow the chain of events:

  • Almost every component in the system has its own goroutine listening for events.
  • All these are event listeners: network manager, page, session, connection, worker, browser...
  • For example: Frame manages its own lifecycle events and deals with its mainframe's and child frames' events.
  • Every component is an executor. So, when you're debugging, it feels as if CDP calls each one of them almost randomly.

For example, when you want to debug something:


Here's an experimental project. It's only for experimenting with CDP, and learning about how it works. I put it here only for reference.

@inancgumus inancgumus added enhancement New feature or request idea 💡 labels Nov 25, 2021
@inancgumus
Copy link
Member Author

@robingustafsson @imiric WDYT?

I'm not saying that we should do this now :)

@imiric
Copy link
Contributor

imiric commented Nov 25, 2021

As we discussed over Slack, it would be good to abstract away the CDP event loop in a separate package, so that we can start cleaning up common. Listening for CDP events and dispatching it to the rest of the system should ideally happen from a single place.

I don't think we can move to a single event loop, as we need an internal one for navigation and other events, but extracting the CDP one should be straightforward, conceptually at least if not practically 😅

I'll let Robin assign the priority for this :) I think we should fix the currently open major issues and tackle this ASAP after that and before implementing new features, since it will help us troubleshoot any other issues more easily.

@robingustafsson
Copy link
Member

I support any effort to make the code more easily debuggable and long-term maintainable 🙂

Priority wise I'd put this somewhere in the second half of the prio 1 issues...but I still have a bunch of prio 1 issues to create in regards to tests and docs and I'd like to see that we implement some of those before embarking on bigger changes to the internals (and it should help having more integration and end-to-end test coverage before making these kinds of changes as well).

On a related note, I've also many times wished there was a way to separate out a "trace" of log messages belonging to a particular JS API call (eg. page.goto(...) etc) and then having ways to visualize the log messages produced in different goroutines (why I originally added the goroutine ID to log messages) as part of that trace (like a tree view of logs or by using color coding or something like that). Something like what's presented in this paper would be awesome IMO: https://www.cs.ubc.ca/~bestchai/papers/tosem20-shiviz.pdf

@grafana grafana locked and limited conversation to collaborators Dec 6, 2021
@inancgumus inancgumus converted this issue into discussion #142 Dec 6, 2021
@inancgumus inancgumus added the events CDP or internal events related. label Oct 18, 2022
@inancgumus inancgumus closed this as not planned Won't fix, can't repro, duplicate, stale Feb 27, 2023

This issue was moved to a discussion.

You can continue the conversation there. Go to discussion →

Labels
enhancement New feature or request events CDP or internal events related. idea 💡
Projects
None yet
Development

No branches or pull requests

3 participants