threema-ch / threema-msgapi-sdk-python

Threema Gateway Message SDK for Python
https://gateway.threema.ch
MIT License
50 stars 16 forks source link

Connection reuse #29

Closed dbrgn closed 7 years ago

dbrgn commented 7 years ago

When sending

Example:

from threema.gateway import Connection              
from threema.gateway.e2e import TextMessage

connection = Connection(
    identity='*YOUR_GATEWAY_THREEMA_ID',
    secret='YOUR_GATEWAY_THREEMA_ID_SECRET',
    key='private:YOUR_PRIVATE_KEY',
    blocking=True,
)

with connection:
    TextMessage(
        connection=connection,
        to_id='ECHOECHO',
        text='test1'
    ).send()
    TextMessage(
        connection=connection,
        to_id='ECHOECHO',
        text='test2'
    ).send()

with connection:
    TextMessage(
        connection=connection,
        to_id='ECHOECHO',
        text='test3'
    ).send()

I can send test1 and test2, but test3 fails:

  ...
  File "/mnt/data/Data/.virtualenvs/test/lib/python3.6/site-packages/aiohttp/client.py", line 585, in __iter__
    resp = yield from self._coro
  File "/mnt/data/Data/.virtualenvs/test/lib/python3.6/site-packages/aiohttp/client.py", line 163, in _request
    raise RuntimeError('Session is closed')
RuntimeError: Session is closed
lgrahl commented 7 years ago

I doubt this is limited to the blocking API or have you tested that?

dbrgn commented 7 years ago

No I didn't. But you could be right :)

lgrahl commented 7 years ago

I don't see how this is a bug. That's intended behaviour. You cannot reuse a connection after usage within a with block. The same applies to aiohttp itself and of course also to files:

f = open('.gitignore', 'rb')
with f:
    print(f.read())

with f:
    print(f.read())
[...]
ValueError: I/O operation on closed file.
dbrgn commented 7 years ago

That's semantically different. A file is consumed with an iterator/cursor. A connection just specifies how to connect to a resource.

Intuitively I would expect the __enter__ of the context manager to open a connection and the __exit__ to close it.

lgrahl commented 7 years ago

No, there's no difference in that example. Maybe, it would help you to imagine that the opened file is a stream and not a regular file. The point is that the file is being closed when the with block exits regardless of what I call inside of the block. It could be reopened for the second with block but it isn't. And I assume that the reason is: It's just too implicit. If I call open, I open a file explicitly. If I create a session, that session is created explicitly. The same for our connection object (although we don't actually create a real network connection before we need it, but the session).

Furthermore, I see no reason why you would use a with block twice. If you need it more often, for example when using it in a different object, create a context manager for that object. You'd waste resources otherwise.

lgrahl commented 7 years ago

If you really want to create a new session and use the connection instance in a second block, we could find a compromise (adding a method for that). However, if it's just for the sake of reusing the connection instance in a different function, then it's a waste of resources (to close the session) and a workaround for a problem that shouldn't exist.

dbrgn commented 7 years ago

I'm perfectly aware of performance implications. It would just make an easier API :)

I'm OK with not changing the behavior, but then it should be documented, and the error message could be a bit nicer.

lgrahl commented 7 years ago

I agree that documentation should be improved.

What exception would you expect then?

dbrgn commented 7 years ago

Something like RuntimeError('Session closed. A connection cannot be reused!') would be enough :)

lgrahl commented 7 years ago

I cannot change the exception easily without subclassing aiohttp.ClientSession, so I think this will have to do. But I added a note for that.