jsbronder / asyncio-dgram

Higher level Datagram support for Asyncio
MIT License
55 stars 7 forks source link

Suppress errors in DatagramStream.__del__ #19

Closed sei-jmattson closed 9 hours ago

sei-jmattson commented 5 days ago

This is largely a cosmetic recommendation...

DatagramStream can be garbage collected after the asyncio loop is ended (e.g. Ctrl-C in a terminal cli), causing messy output of RuntimeError('Event loop is closed') for each instance.

I'm content to monkey-patch it in my app, but thought I'd drop a note.

## monkey-patch to suppress `RuntimeError: Event loop is closed`

from asyncio_dgram.aio import DatagramStream
from contextlib import suppress

def __del_suppress_errors__(self):
    with suppress(RuntimeError):
        self._transport.close()

DatagramStream.__del__ = __del_suppress_errors__
jsbronder commented 1 day ago

While I understand the annoyance, it's probably best that you monkey patch this. My hesitation is that while this exception is of low value in response to garbage collection triggered by a SIGINT, it is valuable if you're managing multiple event loops or starting/stopping one and expect your DatagramStreams to close first (presumably after alerting the other side). Or even if you just have an expectation that your code provides a way to implement a graceful shutdown.

The "correct" way to deal with your issue is probably to add a signal handler or write a context manager that makes sure the DatagramStreams are correctly closed before garbage collection starts. This is the approach I've taken when using this package. Depending on the context, monkey-patching is probably appropriate when weighed against the amount of work required to implement a graceful shutdown.

That said, I'm open to being otherwise convinced.

  1. https://docs.python.org/3/library/asyncio-eventloop.html#asyncio.loop.add_signal_handler
sei-jmattson commented 9 hours ago

Sounds reasonable. Thanks.