async file in multipart file upload

[tsimoshka]

Summary

Trying to implement support for async files (any AsyncIterable[bytes | str]) multipart file upload. Partially fixes #1620

Checklist

• I understand that this PR may be closed in case there was no previous discussion. (This doesn't apply to typos!)
• I've added a test for each change that was introduced, and I tried as much as possible to make a single atomic change.
• I've updated the documentation accordingly. (not sure if update is needed)

[tomchristie]

Is there an async equivalent to typing.IO?
Note that only support byte streams, not text streams.

(Ie. files must be opened in binary mode, or else we raise an exception.)

[tsimoshka]

Is there an async equivalent to typing.IO?

I don't think so. Since all the asyncio files I know are implemented in 3rd party libraries, there is no need for typing.IO asynchronous alternative in stdlib, I guess.

Note that only support byte streams, not text streams.

But it kinda works in that case. I'm using yield to_bytes(achunk) in arender_data to convert str chunk to bytes. I've updated the test to work with the file in text mode also. Not sure if this is correct aproach =/.

[tomchristie]

Ah yeah, I think the failure case is this?...

• Read file size, used for Content-Length header.
• Open a file on a system using a non utf-8 encoding.
• Write to bytes using utf-8.
• Oops, uploaded data stream didn't match the Content-Length.

[tsimoshka]

Ok, I've updated the code and added a runtime checks for bytes in arender_data and updated tests accordingly.

[tomchristie]

Oh, thanks @tsimoshka 😊

[tsimoshka]

BTW, It looks like content=... is already working with {trio,anyio}.open_file, so the #1620 can be closed if this is merged.

[T-256]

Do we want block loop for non-AsynIterable types?

Suggested change

	if not isinstance(self.file, AsyncIterable):
	for chunk in self.render_data():
	yield chunk
	return
	if not isinstance(self.file, AsyncIterable):
	raise TypeError("Invalid type for file. Only AsyncIterable is supported.")

[tsimoshka]

Correct me if I'm wrong, but won't it break IO[bytes] usage with AsyncClient?

[T-256]

Yes, you're right.

[graingert]

Async generator objects should always be typed as AsyncGenerators so they can be used with contextlib.aclosing

[T-256]

Could it be outside of for loop?

[tsimoshka]

I wanted to check the type of file in FileField.__init__. But I think that it's impossible to check the type of the chunks in AsyncIterable at runtime without iterating. Besides, strictly speaking, nothing stops me in python from doing something like:

import asyncio

async def aiter():
    yield b"bytes"
    yield "some str"

async def main():
    result = []
    async for chunk in aiter():
        result.append(chunk)
    print(result)


asyncio.run(main())

I know that this is a very strange example and it's unlikely that someone will do something like that. So I can check only the first chunk in a loop to ensure it is of the correct type. Something like:

checked = False
async for achunk in self.file:
    if not checked and not isinstance(achunk, bytes):
        raise TypeError(
            "Multipart file uploads must be opened in binary mode,"
            " not text mode."
        )
    checked = True
    yield achunk

But I'm not sure whether it'll significantly impact the performance.

[marcodlk]

Would something like this work?

    file_aiter = self.file.__aiter__()
    try:
        achunk = await aiter.__anext__()
    except StopAsyncIteration:
        return

    if not isinstance(achunk, bytes):
        raise TypeError(
            "Multipart file uploads must be opened in binary mode,"
            " not text mode."
        )

    yield achunk

    async for achunk in file_aiter:
        yield achunk

[tsimoshka]

Yes, it would work, but it wouldn't catch the mixed string/bytes iterators I mentioned here. However, I've modified the code as you suggested, since I agree that handling mixed iterators is an edge/unlikely case in this situation..

[noam-better]

Hello! This looks great, any chance it can be merged soon?

[graingert]

You need to explicitly aclose async generators. contextlib.aclosing was added in 3.10 for this purpose, you'll need to copy in the code to support 3.9 or conditionally use contextlib2.aclosing

[graingert]

Async generator objects should always be typed as AsyncGenerators so they can be used with contextlib.aclosing

[graingert]

Use AsyncGenerator type

[graingert]

Use async with contextlib.aclosing

[graingert]

async with contextlib.aclosing

[tsimoshka]

You need to explicitly aclose async generators. contextlib.aclosing was added in 3.10 for this purpose, you'll need to copy in the code to support 3.9 or conditionally use contextlib2.aclosing

Hey! Thanks for reviewing this.

I won't do this for the following reasons:

• contextlib.closing is not used for synchronous files, and files should be closed outside httpx, if I'm not mistaken. Using contextlib.aclosing for async files would make the API different for sync/async operations, which in my opinion might be confusing for users. I think that using closing/aclosing might be an interesting feature, but if it should be implemented, it needs to be done separately and consistently for both sync/async operations.

• It wouldn't allow simple chunking with an iterator when you wrap anyio/trio/anything else into a simple iterator with bufferization and control the chunk sizes yourself. You could implement a generator in that case, but it would be more verbose, I think.

[graingert]

You need to explicitly aclose async generators. contextlib.aclosing was added in 3.10 for this purpose, you'll need to copy in the code to support 3.9 or conditionally use contextlib2.aclosing

Hey! Thanks for reviewing this.

I won't do this for the following reasons:

* `contextlib.closing` is not used for synchronous files, and files should be closed outside httpx, if I'm not mistaken. Using `contextlib.aclosing` for async files would make the API different for sync/async operations, which in my opinion might be confusing for users. I think that using `closing/aclosing` might be an interesting feature, but if it should be implemented, it needs to be done separately and consistently for both sync/async operations.

* It wouldn't allow simple chunking with an iterator when you wrap anyio/trio/anything else into a simple iterator with bufferization and control the chunk sizes yourself. You could implement a generator in that case, but it would be more verbose, I think.

I'm not talking about using aclosing on the files, just the async generators. The AsyncIterator from the file doesn't need to be closed

[tsimoshka]

You need to explicitly aclose async generators. contextlib.aclosing was added in 3.10 for this purpose, you'll need to copy in the code to support 3.9 or conditionally use contextlib2.aclosing

Hey! Thanks for reviewing this.
I won't do this for the following reasons:

* `contextlib.closing` is not used for synchronous files, and files should be closed outside httpx, if I'm not mistaken. Using `contextlib.aclosing` for async files would make the API different for sync/async operations, which in my opinion might be confusing for users. I think that using `closing/aclosing` might be an interesting feature, but if it should be implemented, it needs to be done separately and consistently for both sync/async operations.

* It wouldn't allow simple chunking with an iterator when you wrap anyio/trio/anything else into a simple iterator with bufferization and control the chunk sizes yourself. You could implement a generator in that case, but it would be more verbose, I think.

I'm not talking about using aclosing on the files, just the async generators. The AsyncIterator from the file doesn't need to be closed

Sorry for the delay. I've added aclosing as requested. To support Python versions prior to 3.10, I've restored the previously removed httpx/_compat.py file, though I'm not certain this is the best approach.