search

jfhbrook / pyee

A rough port of Node.js's EventEmitter to Python with a few tricks of its own
https://github.com/jfhbrook/public
MIT License
371 stars 39 forks source link

AsyncIOEventEmitter should keep a reference to scheduled futures #120

Closed ofacklam closed 1 year ago

ofacklam commented 1 year ago

The documentation for asyncio.ensure_future indicates:

Important See also the create_task() function which is the preferred way for creating new Tasks. Save a reference to the result of this function, to avoid a task disappearing mid-execution.

Currently, this doesn't seem to be the case in AsyncIOEventEmitter, which to my understanding only adds a "done" callback, but doesn't keep the future's reference any longer.

jfhbrook commented 1 year ago

I think that assigning to fut and having callback close around it might be enough. I really don't want to get in the business of storing futures in a set and tests are consistently behaving so I'm going to hold off on anything more obvious, but if there ends up being a concrete bug I'm happy to take a closer look.

auxsvr commented 1 year ago

When _emit_run finishes, fut is out of scope, so the future may be garbage-collected. This causes a bug in playwright that loses events, as the corresponding futures vanish before they're fully executed and the corresponding error message appears when the program finishes. A solution is to store the futures in a file-level variable, then use the callback to remove them once finished.

hidaris commented 1 year ago

Is there any further conclusion here?

jfhbrook commented 1 year ago

I'm currently pushing a fix in v9.1.1 - give it a shot, and if you still have issues let me know.

elacuesta commented 1 year ago

For the record, we're still observing this issue with pyee==11.0.0 (see https://github.com/scrapy-plugins/scrapy-playwright/issues/188 & https://github.com/scrapy-plugins/scrapy-playwright/issues/233). I'm mostly leaving this message to keep track of this, at the time I'm not able to provide a reproducible example using only pyee and I'm well aware that the issues I mentioned add two layers of dependencies :disappointed:.