Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Disconnect in destructor causes asyncio errors #1133

Closed
dmig opened this issue Mar 13, 2019 · 3 comments
Closed

Disconnect in destructor causes asyncio errors #1133

dmig opened this issue Mar 13, 2019 · 3 comments

Comments

@dmig
Copy link

dmig commented Mar 13, 2019

Python 3.6.7 x64, Ubuntu 18.10, latest code from master branch.

Nailed down an error:

2019-03-13 20:23:03,672 E asyncio Task was destroyed but it is pending!
task: <Task pending coro=<_recv_loop() running at lib/python3.6/site-packages/telethon/network/mtprotosender.py:416> wait_for=<Future pending cb=[<TaskWakeupMethWrapper object at 0x7fc534ab2a98>()]>>
Exception ignored in: <coroutine object _recv_loop at 0x7fc534a71f10>
Traceback (most recent call last):
  File "lib/python3.6/site-packages/telethon/network/mtprotosender.py", line 416, in _recv_loop
    body = await self._connection.recv()
  File "lib/python3.6/site-packages/telethon/network/connection/connection.py", line 119, in recv
    result = await self._recv_queue.get()
  File "/usr/lib/python3.6/asyncio/queues.py", line 169, in get
    getter.cancel()  # Just in case getter is not done yet.
  File "/usr/lib/python3.6/asyncio/base_events.py", line 580, in call_soon
    self._check_closed()
  File "/usr/lib/python3.6/asyncio/base_events.py", line 366, in _check_closed
    raise RuntimeError('Event loop is closed')
RuntimeError: Event loop is closed
2019-03-13 20:23:03,674 E asyncio Task was destroyed but it is pending!
task: <Task pending coro=<_send_loop() running at lib/python3.6/site-packages/telethon/network/mtprotosender.py:378> wait_for=<Future pending cb=[<TaskWakeupMethWrapper object at 0x7fc534ab2ca8>()]>>
2019-03-13 20:23:03,674 E asyncio Task was destroyed but it is pending!
task: <Task pending coro=<Connection._send_loop() running at lib/python3.6/site-packages/telethon/network/connection/connection.py:131> wait_for=<Future pending cb=[<TaskWakeupMethWrapper object at 0x7fc534ab2cd8>()]>>
2019-03-13 20:23:03,674 E asyncio Task was destroyed but it is pending!
task: <Task pending coro=<Connection._recv_loop() running at lib/python3.6/site-packages/telethon/network/connection/connection.py:149> wait_for=<Future pending cb=[<TaskWakeupMethWrapper object at 0x7fc534ab2af8>()]>>
2019-03-13 20:23:03,674 E telethon.network.connection.connection Unexpected exception in the send loop
Traceback (most recent call last):
  File "/usr/lib/python3.6/asyncio/queues.py", line 167, in get
    yield from getter
GeneratorExit

During handling of the above exception, another exception occurred:

Traceback (most recent call last):
  File "lib/python3.6/site-packages/telethon/network/connection/connection.py", line 131, in _send_loop
    self._send(await self._send_queue.get())
  File "/usr/lib/python3.6/asyncio/queues.py", line 169, in get
    getter.cancel()  # Just in case getter is not done yet.
  File "/usr/lib/python3.6/asyncio/base_events.py", line 580, in call_soon
    self._check_closed()
  File "/usr/lib/python3.6/asyncio/base_events.py", line 366, in _check_closed
    raise RuntimeError('Event loop is closed')
RuntimeError: Event loop is closed
Exception ignored in: <coroutine object Connection._send_loop at 0x7fc534a71fc0>
Traceback (most recent call last):
  File "lib/python3.6/site-packages/telethon/network/connection/connection.py", line 141, in _send_loop
    self.disconnect()
  File "lib/python3.6/site-packages/telethon/network/connection/connection.py", line 96, in disconnect
    self._recv_task.cancel()
  File "/usr/lib/python3.6/asyncio/base_events.py", line 580, in call_soon
    self._check_closed()
  File "/usr/lib/python3.6/asyncio/base_events.py", line 366, in _check_closed
    raise RuntimeError('Event loop is closed')
RuntimeError: Event loop is closed
2019-03-13 20:23:03,676 E asyncio Task was destroyed but it is pending!
task: <Task pending coro=<UpdateMethods._update_loop() done, defined at lib/python3.6/site-packages/telethon/client/updates.py:207> wait_for=<Future pending cb=[<TaskWakeupMethWrapper object at 0x7fc534ab2b28>()]>>

Reason:

# create client, connect, do something
del client
# ... and soon after
loop.close()

Not sure if it's a python issue, maybe a way how GC works or destructor being called. But definitely, all disconnect routines must await their tasks.

For now, the workaround is:

# create the client, connect, do something
client.disconnect()
del client
# ... and soon after
loop.close()

Calling .disconnect() manually helps.

@dmig
Copy link
Author

dmig commented Mar 13, 2019

Update: calling .disconnect() manually helps only on simple testcase. For more complex code with socket server only 0.5s sleep before loop.stop() help to prevent this error.

@dmig
Copy link
Author

dmig commented Mar 13, 2019

Better workaround:

# create the client, connect, do something, disconnect
loop.run_until_complete(asyncio.wait(asyncio.Task.all_tasks(loop))) 
loop.close()

@Lonami
Copy link
Member

Lonami commented Mar 18, 2019

Disconnect in destructor

Thing is disconnect is not called in __del__, but the issue persists. It is very unfortunate that Python's asyncio has no way to Task.cancel() without requiring the loop to run that task again, just to kill it, because in my opinion disconnect should not be asynchronous.

I actually purposedly introduced that change in 9cbc088 in a non-breaking way and now reverting it would be breaking.

The only real reason to disconnect in __del__ (which I emphasize does not happen in your issue) is to properly close everything, including the session file, giving it a chance to save entities to disk, because else a lot of people complain that "it can't find the corresponding entity" (and it's their fault because they are supposed to disconnect).

You can run loop.run_until_complete(asyncio.wait(asyncio.Task.all_tasks(loop))), but it would be trickier for the library, because we don't want to unconditionally run all tasks until their completion. Who knows what the user has in there.

Thoughts?

Lonami added a commit that referenced this issue Mar 21, 2019
Destructors are not guaranteed to run. Despite having good
intentions (saving entities even if the user forgets), it
should be the user's responsability to cleanly close the
client under any circumstances.
Lonami added a commit that referenced this issue Mar 21, 2019
It's the only way to properly clean all background tasks,
which the library makes heavy use for in MTProto/Connection
send and receive loops.

Some parts of the code even relied on the fact that it was
asynchronous (it used to return a future so you could await
it and not be breaking).

It's automatically syncified to reduce the damage of being
a breaking change.
@Lonami Lonami closed this as completed Mar 21, 2019
kingjan123 added a commit to kingjan123/Telethon that referenced this issue Apr 16, 2019
* Change code to recv and handle disconnections

* Update to v1.4.2

* Refetch ChatAction service message to get input users

* Handle TimeoutError on automatic reconnect

* Fix crash on get_messages(ids=InputMessageReplyTo)

* Use InputMessageReplyTo in get_reply_message

This lets bots access to messages other bots sent through replies.

* Replace error and method data with CSV files

* Update code generator to parse CSV files

* Make use of the new method info when generating docs

* Add two MEDIA_*_INVALID to known errors

* Fix _document_by_attribute calls using cond

* Move error capture names to errors.csv

* Remove now unused code to fetch errors from remote host

* Use an enum for methods' usability

* Clarify who can use methods in the documentation

* Update to v1.4.3

* Fix pts initialized as -1 not being considered

* Fix aaee092 to correct entities' offsets on stripping

* Better catch_up behaviour when invalid states are present

* Expect ConnectionError in the send loop

* Fix ConnectionHttp SSL socket wrap (LonamiWebs#1064)

* Never check channel constructor when generating objects

* Slightly smarter search generated docs

* Use decodeURI when loading GET parameters

* Improve autogenerated examples with more cases and synonyms

* Be explicit that phone numbers only work if in your contacts

* Raise TypeError in get_input_peer if access_hash is None

When auto-casting to input peers if the access hash is now None,
the ID will be used to look-up a cached access hash which will
solve some common pitfalls (using full entities when only the
input variant should and could be used).

* Skip core types when parsing .tl files

This avoids the need to comment then every time the file changes.

* Use pathlib.Path in setup.py

* Redirect to first term when pressing enter on docs search

* Except TypeError on empty access hash from 5018879 thoroughly

* Update to layer 89

This breaks edit_2fa

* Prevent caption=None from crashing (LonamiWebs#1071)

* Prevent KeyError in forward_messages

* Prevent common pitfall when pulling new .tl files

* Default to dark theme to avoid screen flashing and fix typo

* Make use of pathlib nearly everywhere (breaks docs gen)

Python 3.6 introduced support for the os.PathLike interface,
which means Python 3.5 did not have it yet and attempting to
use it in os functions would fail. Instead we can use pathlib
for everything, but not all work is done yet.

* Make complete use of pathlib (fix up 8224e5a)

* Update to layer 91

* Update 2fa code to layer 91

* Actually perform all checks in 2fa

* Remove old code to get the hash of a password

* Implement clearing password in edit_2fa

* Use getUsers/getChannels with hash 0 on get_input_entity

* Add new is_bot method to check if the logged-in user is a bot

* Implement click for KeyboardButtonGame

* Revert docs search to use onkeyup or last keystroke is ignored

* Pre-create directory structure when generating docs

This reduces the amount of system calls
from thousands to a few dozens at most.

* Fast-path good known prime in 2fa

* Extend new_algo.salt1 to fix edit_2fa (LonamiWebs#1072)

The salt1 that is sent to the server requires an additional 32 bytes
of random data. It's easy to miss this requirement from reading the 
tdesktop source, because this extension is done in a function called
`ValidateNewCloudPasswordAlgo`.

https://github.com/telegramdesktop/tdesktop/blob/2e5a0e056cdb40d61d487c6062bffe1a835f
6ddd/Telegram/SourceFiles/core/core_cloud_password.cpp#L210-L211

* Add missing file_reference to InputDocument

* Check g when password's prime is known to be good

* Get rid of now broken cache

* Except MessageIdsEmptyError when getting messages by ID

This error may occur even when the IDs are not actually empty.

* Better GetAdminLogRequest example

* Revert "Get rid of now broken cache"

This reverts commit f73ae42.

* Workaround file references by using empty byte strings for cache

They seem to work for now, so until there is a need to update it,
cache will just rely on empty byte strings for the file_reference.

* Update to v1.5

* Return downloaded bytes when file=bytes

* Add TLObject.to_json() for convenience

* Fix get_peer for int which made ab557a8 useless

* Fix UnicodeDecodeError with malformed input on unparse text

* Don't mark the user as connected until successfull connection

The idea behind distinguishing between the user being connected and
the actual connection having been made was to support retries while
the user decided to connect, even if the connection wasn't made yet.

The problem is that _user_connected is used directly to tell whether
the sender is connected or not which will be wrong if an exception
occurs on the initial connection. Since the logic retry isn't used
there we can simply mark as connected once a successfull connection
is made.

* Add friendly method to get admin log (LonamiWebs#952)

* Add more properties to the client to make media more convenient

* Support configuring the reply markup through buttons

* Update documentation to reflect new methods

* Remove full API examples that have a friendly counterpart

* Update to v1.5.1

* Actually make AdminLogEvent work

Ideally this would have been tested before release one commit ago.

* Locally check dialog's offset date (LonamiWebs#1043)

Telegram ignores the parameter if it's alone.

* Support answering inline queries with empty text (LonamiWebs#1053)

* Support passing media objects to inline results (LonamiWebs#1053)

* Don't raise inside __del__ (LonamiWebs#1073)

* Add new takeout method

* Update documentation with takeout, add new known errors

* Update to v1.5.2

* Fix some open calls were not being made through pathlib

This was causing the documentation to fail to build under Python 3.5.

* Tidy examples and update licenses

* Support get_peer on participants and clarify some strings

* Expect UserEmpty on get_input_entity

* Add hide_via to InlineResult.click (LonamiWebs#1084)

* Make logger fully configurable (LonamiWebs#1087)

* Fix-up missing loggers from f271316

* Make Message.edit respect preview and buttons

* Fix send_message with buttons not always returning ReplyMarkup

* Fix and update links in the documentation

* Fix resolve_bot_file_id for photos

* Add yet another missing file_reference

* Fully support email in edit_2fa

* Update to v1.5.3

* Add more missing loggers parameters LonamiWebs#1089

* Document more RPC errors

These two occur when replying to an inline query with a photo and
InputBotInlineMessageMediaAuto empty message, and passing URLs to
PNGs that may not be accessible (i.e. 403 from pixiv).

* Add supports_streaming to send_file and update docs/errors

* Update to layer 93

* Support Path-like values for thumb in _file_to_media (LonamiWebs#1095)

* Document BANNED_RIGHTS_INVALID

* Register several known mimetypes (LonamiWebs#1096)

* Better error diagnostics and fix parsing errors.csv

* Add missing timezone info in important places

Things like SQLAlchemy work correctly only for timezone-aware datetimes.
The returned TLObjects all have them, but those that are manually created
were missing them, so serializing the state into SQLAlchemy sessions failed.

* Update readthedocs with the new ChatAdminRights

* Fix docs generation on Windows and search bar

* Support any of /sS to focus docs search

* Fix search in private chats when from_user was not given

* Saner timeouts for conversation (LonamiWebs#1099)

* Update to v1.5.5

* Update resolve_bot_file_id to latest layer (LonamiWebs#1100)

It is now possible to replace the dummy image with None.

* Fix a couple of inconsistencies in the public interface (LonamiWebs#1102)

* Create `_NOT_A_REQUEST` when needed. Currently, modifications    
  in the raised exception would be "global".    
* `retries` parameters were actually attempts. This has been fixed    
  to actually be the amount of retries, so 0 now means don't retry.    
* Helper function to deal with retries instead of using a range with    
  different styles every time.

* Tiny style fixes

* Update misleading error message

* Correctly proxy __class__ in TakeoutClient for LonamiWebs#1103 (LonamiWebs#1105)

* Improve TakeoutClient proxy and takeout functionality (LonamiWebs#1106)

* Fix accessing conversation messages that arrived too early

* Support MessageMedia in pack_bot_file_id

* Fix incorrect check for reserved data prefix in ConnectionTcpObfuscated

* Fix broken connection establishment in ConnectionTcpObfuscated
This regression was introduced in ebde3be.

* Docs: Removed mention of TcpClient from 'Project Structure'

*  Initial implementation of MTProxy support (LonamiWebs#1107)

* Detail docstring about the 'proxy' parameter in TelegramClient

* Fix get_peer_id not working with InputPeerSelf

* Update URLs in assistant and except message deletion errors

* Support sending bytes/stream as photos (improved _get_extension)

Letting _get_extension work for photos even when the only information
available is a stream or a few bytes allows sending arbitrary bytes
as photos, if force_document was not set to True explicitly.

* Expose phone and phone_code_hash in sign_up

* Fix incorrect sending of DC id when connecting to MTProxy

* Raise ImportError and not ValueError when sqlite3 is missing

Excepting ValueError when creating the SQLiteSession could hide
other errors (e.g. using a newer session file on an older version
of the library). Instead, the original error is raised, as if
sqlite3 was being imported within its __init__ method.

* Check dc_id in resolve_bot_file_id

* Resize photos when sending files if needed

* Further document automatic photo resizing

* Lower the maximum amount of requests per container

* Fix get_input_media for InputFile

It was actually using FileLocation, which had the invalid expected
type for what was being returned. Instead it should have been this
other type.

In addition, more parameters are passed so that the method can have
all the information it needs to correctly cast the type.

* Document common usages for upload_file

* Fix login failure due to wrong argument name (LonamiWebs#1109)

* Except RpcMcgetFailError as call fail and sleep on server errors

This one in particular may happen when iterating over messages,
perhaps more likely to happen if the group was created in a
different data center.

A small sleep of a few seconds also greatly increases the
chances of the error going away.

* Fix photo resizing not working for images with alpha channels

* Document new errors and limits for inline results

* Fix order of inline results not being preserved

* Clearer error on send_file(chat, invalid type)

* Fix sending PNGs without alpha channel, edit size typo

* Fix Connection abstraction leak

* Provide a blanket implementation for _init_conn

* Update to layer 95

* Get rid of broken JSON migration support (LonamiWebs#1113)

* Fix sending albums with bot file IDs

* Cleanup converting to input media in send album

* Clean up iter_messages with reverse=True

* Create a new RequestIter ABC to deal with iter methods

This should make it easier to maintain these methods, increase
reusability, and get rid of the async_generator dependency.

In the future, people could use this to more easily deal with
raw API themselves.

* Implement iter_messages with search

* Make iter_messages use a common message iterator

* Fix setting batch size

* Fix updating offset

* Fix searching messages in reverse

* Don't make iter_messages a coroutine function

* Implement iterator over message by IDs

* Add method to collect RequestIter into TotalList

* Remove code to syncify async generator functions

* Use RequestIter in the dialog methods

* Fix RequestIter not setting TotalList.total in collect()

* Use RequestIter in chat methods

* Remove async_generator from dependencies

* Simplify filling RequestIter's buffer

* Remove weird map chr range from aggressive iter_participants

* Remove batch_size parameter from iter_messages

It was only useful for testing purposes, and no other methods
exposed this kind of parameter (but they still use it).

* Remove the "private" _total parameter

* Handle negative limits gracefully in async generators

We rely on >= 0 for setting the batch size to use (which must
be valid), so it makes sense to make negative limits equal 0.

This is similar to how asyncio.sleep(negative) sleeps 0 seconds,
despite the fact that time.sleep(negative) fails.

* Use constants for chunk sizes, remove irrelevant TODO

* Add RequestIter._next__ for synchronous iteration

* Update documentation, errors and add TODOs

* Treat users "kicking themselves" as leaving (LonamiWebs#1116)

* Update to v1.6

* Fix RequestIter.__next__ propagating StopAsyncIteration (LonamiWebs#1117)

* Fix iter_messages with ids= not being a list

* Actually fix ids= not being a list, bump 1.6.1

* ValueError fix for IOBase files (LonamiWebs#1119)

* Update docs, usability and errors for all methods

* Deal with usability in methods that hit flood wait

* Fix iter_participants in non-channels

* Handle hachoir metadata more gracefully, bump 1.6.2

Since bf11bbd _get_extension supports plenty more things,
which hachoir cannot deal with. Add some extra safety checks.

* Treat arguments with _until or _since in their name as dates

* Prevent pillow from closing non-exclusive fps (LonamiWebs#1121)

* Fix upload_file not seeking streams back

This would cause issues in _cache_media since utils.is_image fails
in the second pass (it respects the stream's position, and the user
may rightfully pass a stream that should be read only from one pos).

* Reduce calls to utils.is_image

* Fix handling message payloads that are too large

* Implement different mtproto proxy protocols; refactor obfuscated2

* Clarify some docstrings

* Don't set self._state when checking if logged in

This essentially made catch_up useless after .start().
cc LonamiWebs#1125 since this affects catch_up.

* Work around message edits arriving too early in conversations

* Rework class hierarchy, try to DRY more

* Revert non-related change

* Use `issubclass` instead of direct class comparison

* Fix-up file to media calls from edit (from 3d72c10)

* Fix DialogsIter not passing the client to the built objects

* Minor doc updates, fixes and improvements

In particular, removed code which no longer worked, made light
theme easier on the eyes, added slight syntax highlightning,
and fixed search for exact matches.

* Update docs for send_file/timeouts and add new known error

* Workaround LonamiWebs#1124 (Telegram ignoring offset_date)

* Workaround LonamiWebs#1134 by early checking if proxy closes connection

* Don't disconnect() on __del__ (LonamiWebs#1133)

Destructors are not guaranteed to run. Despite having good
intentions (saving entities even if the user forgets), it
should be the user's responsability to cleanly close the
client under any circumstances.

* Revert disconnect() to be async again (LonamiWebs#1133)

It's the only way to properly clean all background tasks,
which the library makes heavy use for in MTProto/Connection
send and receive loops.

Some parts of the code even relied on the fact that it was
asynchronous (it used to return a future so you could await
it and not be breaking).

It's automatically syncified to reduce the damage of being
a breaking change.

* run_until_disconnected() should disconnect() on finally

* Properly document all optional dependencies

* Prevent double autoreconnect like LonamiWebs#1112

* Fix run_until_disconnected's call to disconnect

* Safer auto reconnect to prevent double connect

* Fix CallbackQuery.edit for messages via inline queries

* Update to layer 97

* Document that now incoming private messages can be revoked

* Fix UserUpdate not working for typing updates

The code to deal with the right chat action was there,
but the updates that could trigger that code path were
never checked.

* Fix UserUpdate in chats

* Fix-up user_id for UserUpdate

* Create a new in-memory cache for entities (LonamiWebs#1141)

* Use the new in-memory entity cache

This should avoid a disk access every time an input entity
is needed, which is very often. Another step for LonamiWebs#1141.

* getDifference if the event's chat was not found (WIP)

* Except IOError and not ConnectionError

PySocks' errors do not subclass ConnectionError so the library
was unnecessarily logging them as unexpected, when any IOError
was actually expected.

* Propagate the last error on reconnect, not always ConnectionError

* Implement _load_entities for all events

Follow-up of c902428
This means that now input_chat (and even chat_id) and
similar can be safely used, without needing get_input

* Load entities for new via_bot property and forward

* Don't clear pending_ack on disconnect

Upon reconnecting, we must make sure to send all `pending_ack`,
or Telegram may resend some responses (e.g. causing duplicated
updates).

* Call catch_up on reconnect (WIP for LonamiWebs#1125)

* Fix RequestIter.__anext__ without __ainit__ (LonamiWebs#1142)

* Fix ._chat_peer could be None in Event.filter()

* Fix MessageRead had blacklist_chat=None and not False

This was causing the checks against chats to fail. In addition
to that, before setting the attribute, it is now casted to bool
to prevent more issues like this in the future (or if users
use non-boolean values).

* Handle disconnection errors more gracefully in background tasks

* Fix CallbackQuery.edit for normal queries (0b4d649)

* Document misleading message.out value for events.MessageEdited

* Fix resize if needed not seeking back for image = bytes

* Update forward_messages to use _get_response_message

* Let forward_messages work with messages from different chats

Previously it would take the first chat it found and use the IDs
of all messages, even if they belonged to different chats, resulting
in unexpected messages to be forwarded.

Another solution would be to check that all the chats are the same,
but this solves the issue more nicely and makes it more powerful.

* Fix downloading contacts (LonamiWebs#1147)

* Better get_input_entity code flow

Plenty of unnecessary exceptions were being raised and caught when
the input parameters were already correct. Furthermore, since
8abc7ad, the in-disk cache was no
longer being used (so using usernames always reached out to memory).

* Prevent download_profile_photo from downloading arbitrary files

First of all, because it shouldn't be doing that. Second, it was
buggy and was passing the tuple returned by get_input_location to
download_file which doesn't accept tuples (instead it should be
passed the photo object so that download_file could return dc_id
and input file location itself).

* Fix mimetype for mp3 files

It was incorrectly audio/mp3, when audio/mpeg is the correct one.
This was causing sending mp3 voice notes to not work.

* Remove unwanted autocast in messages.discardEncryption

* Force fetch sender/chat if only min information was given

* Add new errors to the list of known ones

* Fix seeking on strings from c0828f5 when uploading files

* Fix _get_response_message when integers are given LonamiWebs#1151

* Fix sending albums not returning the sent messages (LonamiWebs#1151)

* Add missing files for the previous commit

* Save pts and date in a tuple for immutability

This way it is easy and cheap to copy the two required values
to all incoming updates in case we need to getDifference since
the previous pts/date to fetch entities.

This is still a work in progress.

* Expose grouped parameter in forward_messages (default True)

* Smarter grouping behaviour when forwarding messages

* Fix catch_up pts loading and remember pts per update

* Add new action method to easily send chat actions like typing

* Reduce __enter__/__exit__ boilerplate for sync ctx managers

* Fix-up new sync __enter__ not handling the client directly

* Rename grouped to as_album in forward_messages

Public comments regarding this change can be found at:
LonamiWebs@cf3a4bc

* Fix CallbackQuery ignoring func

* Fix loading initial pts/date could be None (LonamiWebs#1153)

* Fix iter_dialogs missing many dialogs
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

No branches or pull requests

2 participants