jupyterlite / pyodide-kernel

Python kernel for JupyterLite powered by Pyodide
https://jupyterlite-pyodide-kernel.readthedocs.io/en/latest/_static/
BSD 3-Clause "New" or "Revised" License
47 stars 26 forks source link

Use `coincident` if `crossOriginIsolated`, `comlink` otherwise #126

Closed jtpio closed 4 months ago

jtpio commented 4 months ago

Looking into https://github.com/jupyterlite/jupyterlite/issues/1424.

coincident uses a SharedArrayBuffer here: https://github.com/WebReflection/coincident/blob/01cc8a1e90abb360deb9eabd7773ef4355fcbc3d/esm/index.js#L82

However in environments that are not crossOriginIsolated, usings coincident on the worker side leads to the following issues:

TypeError: [object Int32Array] is not a shared typed array.

So this PR takes the following approach:

Fixes https://github.com/jupyterlite/pyodide-kernel/issues/127

github-actions[bot] commented 4 months ago

lite-badge :point_left: Try it on ReadTheDocs

jtpio commented 4 months ago

Current status, after a naive replacement of postMessage by coincident:

image

bollwyvl commented 4 months ago

Maybe we need to be using coincident/structured?

jtpio commented 4 months ago

Right, I'll have a closer look.

jtpio commented 4 months ago

This seems to be caused by coincident using a SharedArrayBuffer here: https://github.com/WebReflection/coincident/blob/01cc8a1e90abb360deb9eabd7773ef4355fcbc3d/esm/index.js#L82

It seems to be working fine when serving the site with right headers, for example with: npx static-handler --cors --coop --coep --corp ./build/docs-app

I thought coincident would default to the comlink behavior when SharedArrayBuffer are not available, but it looks like it's not the case.

jtpio commented 4 months ago

Switching to coincident on the worker side does seem to be fixing https://github.com/jupyterlite/jupyterlite/issues/1424.

coincident-widget-error.webm

I thought coincident would default to the comlink behavior when SharedArrayBuffer are not available, but it looks like it's not the case.

Maybe we need to clearly separate the case when SharedArrayBuffer are available on the page from when they are not. Meaning using coincident when they are, and comlink when they are not (on both the worker and the main threads).

cc @martinRenou in case you have some input on this

martinRenou commented 4 months ago

Meaning using coincident when they are, and comlink when they are not (on both the worker and the main threads).

That's unfortunate, but if it's the only solution than let's go for it.

coincident is really taking the burden of implementing a proper communication protocol using SharedArrayBuffers, so it's really useful to us in that case. That's unfortunate it cannot work in an asynchronous fashion though, closer to the comlink approach.

martinRenou commented 4 months ago

Thanks for looking into it by the way 👍🏽 I'm swamped with other projects. We should apply similar changes to jupyterlite/xeus once we have a proper solution.

bollwyvl commented 4 months ago

Yeah... As realizing the merit of the SAB is getting more and more tenuous, it looks like it needs to be entirely opt-in.

On Wed, Jul 17, 2024, 03:42 martinRenou @.***> wrote:

Thanks for looking into it by the way 👍🏽 I'm swamped with other projects. We should apply similar changes to jupyterlite/xeus once we have a proper solution.

— Reply to this email directly, view it on GitHub https://github.com/jupyterlite/pyodide-kernel/pull/126#issuecomment-2232762772, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAALCRHRBWNJL2KS5OEY7I3ZMYVBBAVCNFSM6AAAAABKYZQMG6VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDEMZSG43DENZXGI . You are receiving this because you commented.Message ID: @.***>

jtpio commented 4 months ago

Looks like we could keep the current logic of going over the SharedArrayBuffer if they are available, and default to the service worker when they are not.

If the SharedArrayBuffers are available, we use coincident on both the main thread and the worker. If they are not we use comlink.

jtpio commented 4 months ago

I'll push a first draft towards that direction and will clean things up a bit afterwards.

jtpio commented 4 months ago

Thanks for the review!

martenrichter commented 3 days ago

This PR caused the breakage of interact, see: https://github.com/jupyterlite/pyodide-kernel/issues/143