MrYsLab / pymata-aio

This is the second generation PyMata client.
https://github.com/MrYsLab/pymata-aio/wiki
GNU Affero General Public License v3.0
155 stars 51 forks source link

Do not destroy existing loops #102

Closed DaAwesomeP closed 4 years ago

DaAwesomeP commented 4 years ago

Similar to #94 in that this will allow pymata to run and fail/quit without affecting the rest of the program. This spawned from https://github.com/home-assistant/home-assistant/pull/24014.

This is all theoretical, and my understanding of Python async loops may be flawed somewhere.

When failing, pymata should only cancel the tasks it started and should only stop or close the loop if it plans to exit (port_discovery_exceptions is false). So instead of:

try:
    loop = self.loop
    for t in asyncio.Task.all_tasks(loop):
        t.cancel()
    loop.run_until_complete(asyncio.sleep(.1))
    loop.stop()
    loop.close()
    if self.port_discovery_exceptions:
        raise RuntimeError
    else:
        sys.exit(0)
except RuntimeError:
    self.the_task.cancel()
    time.sleep(1)
    # this suppresses the Event Loop Is Running message,
    # which may be a bug in python 3.4.3
    if self.port_discovery_exceptions:
        raise
    else:
        sys.exit(0)

Something like:

try:
    if self.port_discovery_exceptions:
        self.the_task.cancel()        
        raise RuntimeError
    else:
        loop = self.loop
        for t in asyncio.Task.all_tasks(loop):
            t.cancel()
        loop.run_until_complete(asyncio.sleep(.1))
        loop.stop()
        loop.close()
        sys.exit(0)
except RuntimeError:
    self.the_task.cancel()
    time.sleep(1)
    # this suppresses the Event Loop Is Running message,
    # which may be a bug in python 3.4.3
    if self.port_discovery_exceptions:
        raise
    else:
        sys.exit(0)

This is found in the following places:

I have not tested this. I can submit a PR if you would like. I believe this is what is causing the this event loop is already running. and CancelledError issues I am seeing in my code.

Also from looking at it, start and start_aio could probably be merged if start just spawned start_aio as a task, but I am probably missing something important.

MrYsLab commented 4 years ago

Could you please provide some additional information about the problem you are trying to solve. It sounds as if you are trying to kill a pymata-aio application that is somehow killing a home assistant program. Are these 2 separate applications or a single application sharing a single event loop?

If they are sharing a single event loop, can they run independently of each other, each using its own event loop?

Perhaps you can solve your problem by creating a separate thread?

DaAwesomeP commented 4 years ago

I am trying to use pymata as a library, that is all. It is sharing the same event loop in one thread. I don't want the app to quit or fail if pymata fails to connect—I want to catch the error.

I was receiving the version timeout error, but instead of Home Assistant continuing, pymata destroys the event loop when it encounters an error like this. This is fine if pymata is the only thing using the event loop or if you want to quit when pymata fails, but it makes it impossible to simply catch the failure and continue without it.

I could spawn another thread, but that kind of negates the idea of async in this case, and I don't think threads are necessary. Since only one event loop can run at once, I would need to spawn an additional thread with an additional event loop if pymata insists on having its own event loop.

By modifying other tasks in the event loop, start_aio becomes a destructive function. Is there a reason that pymata insists on cancelling all tasks other than its own when it fails? If this is intentional, it really really should be documented clearly that pymata needs to run in its own thread if you plan to create tasks to run asynchronously alongside pymata.

This wouldn't matter if you are using async functions in a synchronous manner like so:

await do_one()
await do_two()
await start_aio()

In the above example, everything basically runs one after the other. But as soon as you schedule tasks for later with something like call_soon(), the event loop runs things in the order that makes most sense for it and whenever it can. You can't assume anymore that nothing else is on the loop.

They shouldn't really be a need here (I don't think) to run them in separate loops or threads. If running pymata in its own loop is vital to its function, then it would be most intuitive for pymata to spawn an additional thread for itself (or have the option to) since in general I don't think any user is expecting it to fail by cancelling all tasks.

MrYsLab commented 4 years ago

Perhaps I am missing something, but you can run two asyncio programs simultaneously using 2 independent event loops. Here is a link to an example: https://gist.github.com/MrYsLab/454251f75c339558490be50ba282f937

If you look at line 12 of both x and y, x gets the "normal" event loop, and y requests a new event loop.

Start both programs up and if you kill one the other keeps running.

Does this solve your problem?

DaAwesomeP commented 4 years ago

Right, so I tried this exactly (passing asyncio.new_event_loop() to event_loop in pymata) as soon as you mentioned it in the other thread, but pyamta then always fails to connect to the board (times out), when it fails and calls run_until_complete, it generates raises a Python runtime error:

Cannot run the event loop while another loop is running

While you can create multiple event loops, it is still one thread, so only one can run at a time. So, the Home Assistant loop tasks would run and the pyamta ones never run. Yes, it fails without killing everything in this way, but it is not very solid since it is raising the wrong error, and, well, it doesn't work.

My I ask what you insistence/worry is with not destroying the passed loop on failure?

I should note that am having the timeout issue regardless. However, I am seeing it only 50% of the time when using the existing event loop, but 100% of the time when generating a new one as done just now. I created this issue first because killing the loop made it very difficult to debug—all of the task cancelled errors and other issues would come up after, and it looked like an issue on the Home Assistant side. So I'll say again—it should be added to the docs pymata destroys the loop when it errors. This is the first async library I have encountered that works this way.

I reported the loop destruction issue first because it made it extremely difficult to debug what was going on. It looked like the issue was not within pymata because it caused the rest of the program to fail.

MrYsLab commented 4 years ago

As I suggested in issue #94, for what you are trying to do, you should be using pymata-express.. It only closes its own tasks and leaves all others intact.

The latest version of Debian installs python 3.7. If you do not wish to upgrade Debian, then you can use pyenv to install python 3.7 or 3.8 to be active in a single directory while keeping the current version active everywhere else.

I do not wish to modify pymata-aio if it changes behavior for other users and creates a breaking change. If you wish to modify it for yourself, please feel free to clone it and modify it for your needs, but at this time, since pymata-aio is essentially in maintenance mode, I do not wish to incorporate any changes.

If you have any questions, I would be pleased to answer them.

DaAwesomeP commented 4 years ago

As I suggested in issue #94, for what you are trying to do, you should be using pymata-express.. It only closes its own tasks and leaves all others intact.

I will definitely migrate to that then. I did not migrate before because I had implemented most of it in aio.

The latest version of Debian installs python 3.7. If you do not wish to upgrade Debian, then you can use pyenv to install python 3.7 or 3.8 to be active in a single directory while keeping the current version active everywhere else.

I'm in a Home Assistant Docker image on 3.8 now.

I do not wish to modify pymata-aio if it changes behavior for other users and creates a breaking change. If you wish to modify it for yourself, please feel free to clone it and modify it for your needs, but at this time, since pymata-aio is essentially in maintenance mode, I do not wish to incorporate any changes.

Perfectly valid! I was not aware of that. Sorry to bug you so much!

If you have any questions, I would be pleased to answer them.

Thank you for creating and maintaining such an awesome resource! In general the whole thing is very solid and has saved me and I'm sure other people countless hours. Thank you also for being so attentive to issues and changes.