Skip to content

Conversation

@WebReflection
Copy link
Contributor

@WebReflection WebReflection commented Jan 24, 2024

Background

Pyodide requires create_proxy not only for event listeners but also for any JS API that might use a callback that is not instantly invoked:

  • GeoLocation watchPosition and others
  • anything media related with permissions callbacks
  • surely any EventTarget
  • various storages listeners or results
  • crypto operations or FileReader and so on

Pyodide did an excellent job in explaining how to work-around but we're having hard time justifying some inconsistency with events themselves or the fact users need to think about when a callback should be wrapped and where it shouldn't.

Following a few examples of such inconsistencies:

# this works
document.body.onclick = lambda e: print(e.type)

# this doesn't
document.body.addEventListener('click', lambda e: print(e.type))

# it's also not clear why third option should be avoided
document.body.addEventListener('click', lambda e: print(e.type), { "once": True })

# in favor of a more verbose and less standard
pyodide.ffi.create_once_callable
# this also very likely creates a leak with the listener
# as that cannot ever be freed or removed
document.body.addEventListener(
  'click',
  # is this ever auto-removed or would this
  # also need a third { "once": True } argument?
  create_once_callable(
    lambda e: print(e.type)
  )
)

# when intervals are created but their "stop" is passed around
def poll(callback, delay = 100):
  return js.setInterval(callback, delay)

# how is anyone supposed to destroy that callback?
# if create_proxy(callback) is used but nobody can't
# tell when such interval will be cleared?

# P.S. maybe pyodide is smarter here but that
#      sums up the point: it's hard to explain
#      to our users when or why some callback
#      should be wrapped, and how to destroy it

There are also cases, such as Promise and generaters, that automatically take care of the callback life span, or create once, or proxy, and we think this might be confusing to reason about from new comers to people coming form JS that know what they are doing but need to learn a foreign API to do what used to be simple.

Bear in mind with MicroPython most of these issues don't exist, so we also have inconsistent Python behavior for, again, what's usually simple to deal with on the Web.

Last, but not least, with the latest ability to load 3rd party JS libraries, the error around callbacks that are passed within 3rd party APIs and nobody can dictate what happens with those callbacks internally and when or why a proxy or a once should be used, we decided to have a conversation and explore possible solutions ... and this MR is the result of such exploration.

Proposal

We cannot possibly patch all DOM APIs that use callbacks or all 3rd party libraries that accept callbacks as argument so that I've had the insane idea of patching the source/root of the problem: the usage of call and apply that, whenever a context is meant to be passed or the amount of arguments is variable, are the most basic primitive any library could use, including Pyodide.

We understand such patch might feel wrong and we'd love to never need to do so, but as our users and the DX or UX of the project is something we really care about, we decided to try at least to see if making this feature opt-in and for Pyodide only, could somehow actually work well.

This MR hence propose the following:

  • if a script config contains experimental_create_proxy = "auto", apply once a patch for call and apply only on main and only if the interpreter is Pyodide
  • the patch, if ever applied on main, simply returns native call or apply functionality unless the Python code is running ... meaning:
    • there shouldn't be ever relevant performance issues as the only extra work is a boolean check
    • only when run(pythonCode) or runAsync(pythonCode) or runEvent(...) such flag is switched to true
    • the patch simply looks for typeof arguments that are function and an instanceof PyProxy, keeping untouched any other kind of argument
    • the FinalizationRegistry takes care of freeing the memory as soon as not needed anymore:
      • there's no need to think about when a callback should be destroyed
      • there's no need to think about create_once_callable VS create_proxy
      • the JS' GC will take care of never bloating the memory and .destroy() those callbacks transparently
    • the patch is never meant to be on the way for either our code, 3rd party libraries, or pyodide itself
    • the patch would never bother MicroPython projects or any code in workers, as the dance there to invoke callbacks on the main thread is different and it doesn't require much thinking about listeners and/or most other cases

This MR is up for discussion but basically allows an explicit opt-in that could be warned as experimental so that both we, pyodide folks, or our users, can provide feedback, tell us if anything degraded performance, and so on so please feel free to join the conversation and tell us what you think about this MR, thank you!

To Be Discussed

The original idea was to also have a way, whenever that desired or needed, to avoid wrapping or copying callbacks via a direct(callback) utility by this module where such utility would be just an identity for all other interpreters but it will instrument pyodide to skip wrapping or checking a specific callback as argument:

from polyscript import direct
from js import document
from js_modules import foreign_thing

foreign_thing.execute('type', direct(lambda e: print(e.type)));

This would be a probably welcome escape hatch for those specific cases where no copy is ever desired but ideally this could be instead provided by pyodide if they'll ever decide that using GC to automatically handle memory is a possible way forward.

@WebReflection WebReflection force-pushed the create_proxy branch 4 times, most recently from c038ec5 to 0f1bd33 Compare January 26, 2024 17:28
@WebReflection
Copy link
Contributor Author

To whom it might concern, I am now more convinced than ever this feature should land in ASAP but it should be behind a very explicit experimental_* config flag, so that it's clear that experimental_create_proxy = "auto" will be available only for people that dares using it, the fact its name could change, or its features be never part of our stable release.

With all this kept in mind, I think it would be more than beneficial to have this flag in so that we can actually, even internally, make any decision about this being a good thing to do, as a patch, or the worst one ever.

@WebReflection WebReflection merged commit e3f13d1 into main Jan 26, 2024
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