samschott / desktop-notifier

Python library for cross-platform desktop notifications
https://desktop-notifier.readthedocs.io
MIT License
81 stars 8 forks source link

Lazily instantiate asyncio event loop; use currently running asyncio event loop if there is one #150

Open michelcrypt4d4mus opened 1 week ago

michelcrypt4d4mus commented 1 week ago

As it is currently stands instantiating DesktopNotifierSync() results in an attempt to create a new asyncio event loop. This is not ideal if

  1. desktop-notifier library is being used in a larger project that is trying to do its own event loop management
  2. DesktopNotifierSync is eagerly instantiated when the application is launched (my app was instantiating `DesktopNotifierSync() at launch time as more or less a singleton)

Changes

This PR contains a small change that:

  1. makes DesktopNotifierSync's event loop instantiation lazy: it only bothers to find or construct an event loop once send() or similar is called by the application
  2. attempts to use the currently executing event loop if there is one: there are some issues with this though, see below.

Theoretically this change would allow someone to use DesktopNotifierSync in a context where some other piece of code has already launched an asyncio event loop. Of course that might be of limited utility (if the app is already running an event loop why would you use DesktopNotifierSync as opposed to just doing it all asynchronously?) but given that i can see a situation where someone using the lib might want to do it (for instance let's say the event loop management is done by some other library they are relying on, which is more or less my use case) but it is admittedly pretty marginal.

Issues

Unfortunately in my particular context of a Twisted networking app the code in this PR doesn't actually work. It just hangs indefinitely waiting for future.result(). i'm pretty sure (though not 100% sure) that this is the fault of Twisted which is a 20 year old networking library that implemented asynchronicity in python literally decades before asyncio even existed and as a result hates it when you try to do any thread management outside of its purview but i could just be making a mistake... either way I figured it might be worth opening this PR just because lazy instantiation of DesktopNotifierSync._loop could prevent an event loop collision should someone attempt to use DesktopNotifierSync in a situation with a running event loop but without a twisted.reactor trying to manage threading and everything else.

To get DesktopNotifierSync.send() to work in my particular context I had to switch to using asyncio.create_task() instead of asyncio.run_coroutine_threadsafe() and then just discard the result of the coroutine via return None (branch comparison view for these changes). That's obviously less than ideal but at least it proves that it can be done.

UPDATE: Example

This short block of code shows how this works in a situation where the event loop is created outside of desktop-notifier:

import asyncio
from desktop_notifier.sync import DesktopNotifierSync
from time import sleep

loop = asyncio.new_event_loop()
print("Created event loop, sleeping...")
sleep(1)
print("Triggering notification...")

notifier = DesktopNotifierSync()
notifier.send(title='Nasir Jones', message='The World is Yours')
print("Triggered notification, starting event loop!")
loop.run_forever()

though i haven't tested trying to use DesktopNotifierSync in a situation where the event loop has been both created and started via loop.run_forever() or similar.

codecov-commenter commented 1 week ago

Codecov Report

Attention: Patch coverage is 87.50000% with 2 lines in your changes missing coverage. Please review.

Project coverage is 75.22%. Comparing base (369c9fa) to head (571e2bb). Report is 32 commits behind head on main.

Files Patch % Lines
src/desktop_notifier/sync.py 87.50% 2 Missing :warning:
Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #150 +/- ## ========================================== + Coverage 75.03% 75.22% +0.19% ========================================== Files 8 9 +1 Lines 833 880 +47 ========================================== + Hits 625 662 +37 - Misses 208 218 +10 ``` | [Flag](https://app.codecov.io/gh/samschott/desktop-notifier/pull/150/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=SamSchott) | Coverage Δ | | |---|---|---| | [pytest](https://app.codecov.io/gh/samschott/desktop-notifier/pull/150/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=SamSchott) | `75.22% <87.50%> (+0.19%)` | :arrow_up: | Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=SamSchott#carryforward-flags-in-the-pull-request-comment) to find out more.

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

samschott commented 6 days ago

I'm not entirely sure why you want to use the synchronous version of the API if you already have a running asyncio event loop.

Since you loose all callback functionality and end up blocking other code, the async API seems like the better choice in cases where asyncio.get_running_loop() would return an event loop in the first place.

Though I am not at all familiar with Twisted, which seems to be the targeted use case. Why is it not possible to just use the async API with Twisted?

michelcrypt4d4mus commented 6 days ago

I'm not entirely sure why you want to use the synchronous version of the API if you already have a running asyncio event loop.

the funny thing is that i don't actually - except for one small task i have to run when bootstrapping my application i use the asynchronous version and in fact that's part of why i chose this library, but messing around with that small piece i ended up with this change which might be a small improvement for other people in other situations just bc of the lazy instantiation.

is it not possible to just use the async API with Twisted?

it is and that's exactly what i'm doing (though it is a bit janky).

this PR doesn't really help me out and you should feel free to just close it, though at the same time i don't think there's any downside to checking for a running event loop in a lazy way instead of in an eager way.

michelcrypt4d4mus commented 4 days ago

i cleaned up the linter issue and removed the [WIP] from the title bc i guess this isn't really WIP.

tldr i don't need this PR for my use case but i do think it's slightly nicer to do a lazy instantiation of the event loop and allow for a running loop. feel free to just close if you don't want this change.