pfeodrippe/wally

playwright-java is not thread-safe - wrap?

Opened this issue · 2 comments

vemv commented

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 commented

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.

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