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

playwright-java is not thread-safe - wrap? #5

Open
vemv opened this issue Jun 1, 2023 · 2 comments
Open

playwright-java is not thread-safe - wrap? #5

vemv opened this issue Jun 1, 2023 · 2 comments

Comments

@vemv
Copy link
Contributor

vemv commented Jun 1, 2023

Per https://github.com/microsoft/playwright-java/tree/b8a9d888be987b5ff522b542827d7a46fb8df90a#is-playwright-thread-safe :

Playwright is not thread safe, i.e. all its methods as well as methods on all objects created by it (such as BrowserContext, Browser, Page etc.) are expected to be called on the same thread where Playwright object was created or proper synchronization should be implemented to ensure only one thread calls Playwright methods at any given time.

By coincidence, I encountered that issue before reading that passage: if I tried to spawn multiple browsers in a pmap, it would fail opaquely.

Therefore it seems a good idea for Wally's API to prevent unsafe usage.

One possible approach would be to use ThreadLocals. And another, to use clojure.core/locking.

I might be able to offer a POC PR with a possible API if you'd appreciate that.

Cheers - V

@vemv
Copy link
Contributor Author

vemv commented Jun 1, 2023

I thought of a fairly minimal solution:

(defonce ^:dynamic *page*
  (delay ;; add a delay, so that the thread that sets this value is the most recent one.
         ;; (note that that would require `make-page` to remove its `delay`, to avoid a double `delay` wrapping)
    (doto (ThreadLocal.)
     (.set (make-page)))))

(defn get-page
  []
  (let [^ThreadLocal page (if (delay? *page*)
                           @*page*
                           *page*)]
    (.get page)))

(defn maybe-wrap-page [page]
  (if (instance? ThreadLocal page)
    page
    (doto (ThreadLocal.)
      (.set page))))

(defmacro with-page
  [page & body]
  `(binding [*page* (maybe-wrap-page ~page)]
     ~@body))

I think that this way, Wally's nice API would remain untouched. The only thing that would be lost is that dynamically-bound values wouldn't be conveyed across threads (other threads would perceive an empty value).

But that would be a good thing! Since unsafe usages would be prevented.

@pfeodrippe
Copy link
Owner

@vemv Awesome! Yes, you can open a PR, thank you very much!!

@vemv vemv mentioned this issue Aug 7, 2023
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