aiohttp 2.x closing client request data stream

[thehesiod]

In previous versions of aiohttp, if you passed a stream as the data parameter to an aiohttp request, it would not close the stream after the request, now it is closing it because it's wrapping it in a Payload, and later the write method closes it.

I believe aiohttp should not be closing the stream sent to it. This new behavior caused this issue with aiobotocore: aio-libs/aiobotocore#221 resulting in retries failing.

[AraHaan]

You can backup the stream to a private variable for the retries. Or have a list of the streams for the retries and then remove the closed ones.

[thehesiod]

I can't backup the stream because aiohttp is modifying the stream ref that gets passed in.

[AraHaan]

Hmm other than subclassing ClientSession or some other class you use that does that to override that particular function that closes the stream. I have no other ideas.

[thehesiod]

ya would have to override close, but that's a hack, I still believe aiohttp shouldn't close streams it's passed in like pre 2.x. Would like to hear why that decision was made, or perhaps it was an oversight and this is a bug.

[fafhrd91]

@thehesiod could you point to code that closes stream?

[thehesiod]

sure, stream is wrapped here:

  File "/Users/amohr/dev/third_party/aiobotocore/aiobotocore/endpoint.py", line 194, in _request
    allow_redirects=False)
  File "/usr/local/lib/python3.5/site-packages/aiohttp/client.py", line 616, in __iter__
    resp = yield from self._coro
  File "/usr/local/lib/python3.5/site-packages/aiohttp/client.py", line 220, in _request
    proxy=proxy, proxy_auth=proxy_auth, timer=timer)
  File "/usr/local/lib/python3.5/site-packages/aiohttp/client_reqrep.py", line 96, in __init__
    self.update_body_from_data(data, skip_auto_headers)
  File "/usr/local/lib/python3.5/site-packages/aiohttp/client_reqrep.py", line 248, in update_body_from_data
    body = payload.PAYLOAD_REGISTRY.get(body, disposition=None)
  File "/usr/local/lib/python3.5/site-packages/aiohttp/payload.py", line 57, in get
    return factory(data, *args, **kwargs)
  File "/usr/local/lib/python3.5/site-packages/aiohttp/payload.py", line 184, in __init__
    super().__init__(value, *args, **kwargs)
  File "/usr/local/lib/python3.5/site-packages/aiohttp/payload.py", line 74, in __init__

and closed here:

  File "/usr/local/lib/python3.5/site-packages/aiohttp/client_reqrep.py", line 316, in write_bytes
    yield from self.body.write(writer)
  File "/usr/local/lib/python3.5/site-packages/aiohttp/payload.py", line 197, in write
    self._value.close()

[thehesiod]

botocore expects to be able to rewind the streams between requests, like it does with urllib

[fafhrd91]

ok, I see. this is about files, I thought it was about StreamReader. it was intentional decision, closing files seems like a good idea, and this code simplifies usage of resources. we can remove this code, or we can add some options for skipping close operation.

@asvetlov what do you think?

[thehesiod]

Well about streams in general. For whatever reason botocore wraps bytes passed to it in a byte stream. Ya the decision is if aiohttp should take full ownership of the steam or not. Given aiohttp did not create the steam i think not...But will let you guys decide as long as there's an option to not close it :)

[thehesiod]

Also think about parity with requests/urllib

[fafhrd91]

I think it actually takes ownership. Also it make usage much simpler, otherwise everyone needs manually maintain resources, which is not very ergonomic. You can wrap your bytes stream into custom payload impl, that should be easy.

[thehesiod]

I just verified requests does NOT close the stream as I would have expected. If someone creates the stream they are explicit owners of said stream and should handle closing as they are the only ones that know when it should be closed. Only if they tell aiohttp to close it should it be closed. Otherwise aiohttp cannot know if the caller wants to use the stream again after the request.

[thehesiod]

further this seems to be an undocumented change in behavior: http://aiohttp.readthedocs.io/en/latest/migration.html

[fafhrd91]

I understand your use case, but requests experience is not very relevant, because request is synchronous, in async code it just safer to pass ownership. You get response before your stream get consumed. I believe take ownership is right approach.

@asvetlov @kxepal comments?

[thehesiod]

Could you explain how closing the steam is safer? There really is no difference between asyncio and sync. In either case you can have multiple simultaneous readers against the same stream. There are two cases: Stream was fully read (get end of stream error on re-use), file was partially or not read (get error thrown from aiohttp method). So I'm not sure what this new behavior is preventing. Other than that the object is ref'd and gc'd so will get destroyed. I'm also guessing that most streams used will be seekable (files and mem) and are designed to be reused.

[asvetlov]

I want to resurrect the issue.

[asvetlov]

On one hand @fafhrd91 is right: moving stream ownership to aiohttp is easier and safe in at least 95% of use cases. Otherwise people should use construction like this:

with open(fname) as f:
    async with client.get(url, data=f) as resp:
         await resp.read()

Too many nested context managers to be used properly by average software developer.
On other hand I understand @thehesiod -- sometimes keeping ownership is crucial.

What about adding opt-in parameter for not transferring ownership to aiohttp?

[fafhrd91]

Too many parameters. Custom payload class is 5 lines of code.

-1

[asvetlov]

Yes, it's true.
But passing custom payload instance is not supported now.
Maybe we could register another identity payload type into PAYLOAD_REGISTER for using payload as is without transformation?

@thehesiod might pass custom payload for his needs.
Moreover we could add close=True parameter to IOBasePayload for making life simple without overwhelming main client API.

[fafhrd91]

WTF? https://github.com/aio-libs/aiohttp/blob/master/aiohttp/payload.py#L54

I say no.

[asvetlov]

@fafhrd91 please keep calm. I'm not very familiar with payload API, sorry.
If it already supports Payload instances -- that's pretty fine.
Adding a close parameter with default True value doesn't blow the API at all I believe.
If it could help @thehesiod -- why not?
I'd like making life for aiobotocore maintainers easier if it doesn't make harm to aiohttp itself.

[fafhrd91]

If you are not familiar then why change my decisions? Helping everyone == crappy api.
Again wtf? Open file, give it away. Need again, open again. Compared to transfer time it is nothing

[fafhrd91]

https://github.com/aio-libs/aiohttp/blob/master/aiohttp/payload_streamer.py#L61

[asvetlov]

I don't want to change your decision at all, don't get me wrong.
What I want is finding a way to help aiobotocore.
Say again, I'm not familiar with payload API.
If there is a way to help them -- please make a hint.
Maybe your last link is the hint but personally I don't understand it now.

Let's make a pause.
@thehesiod please describe me why file reopening doesn't work for you. I suggest to do it in aiobotocore tracker for not polluting this thread but not in private emails for keeping history open.

P.S.
I really want to understand the needs of both parts very well first.

[thehesiod]

I have no urgent need for this feature right now as I right now wrap our stream when I send it to aiohttp so when aiohttp calls close it does nothing. This just feels very hacky. If I were to wrap the payload I believe I would still have to do this as aiohttp unconditionally calls close. If calling "close" were a good idea by http libraries you would have thought requests (which has been around for a lot longer) would have done this as well. I still haven't been presented with any evidence what problem this solves, and in fact presented evidence with an issue it created.

[jmehnle]

If this is representative of how the aiohttp project treats its users (first change API, then throw "WTF"s at those who ask for the API change to be optional), then I'll make a mental note to stay clear. Wow.

[asvetlov]

Closing the issue. It is a year old.
aiobotocore works well, changing (or reverting back) the api will break existing code again.

[asvetlov]

@jmehnle thank you for attracting attention to the outdated issue.

[jmehnle]

Better to close this and acknowledge that the last word has been spoken than get people's hopes up. :)

[asvetlov]

@jmehnle sorry, I don't follow your message.
Do you propose something?

[lock]

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a [new issue] for related bugs.
If you feel like there's important points made in this discussion, please include those exceprts into that [new issue].
[new issue]: https://github.com/aio-libs/aiohttp/issues/new