turn MapAsyncIterable into a map_async_iterable AsyncGeneratorFunction

[kristjanvalur]

This is a continuation of pr #197

It completely replaces the class MapAsyncItarable with map_async_iterable() Async Generator Function. This is an inner, undocumented, class and so no documentation changes are necessary.

Also, using a standard AsyncGenerator, we can throw away all of the tests that were created to test the minute details of the old MapAsyncIterable class and just add a couple of tests to verify that an inner aclose() is called in case an error occurs during execution of callback().

aclose() is not a standard part of an AsyncIterator. It is also not required to be called for an AsyncGenerator since an aclose() call will be scheduled to be executed if an async generator gets garbage collected. But it is good programming practice to do it as part of a finally clause so that it hapens promptly in case of an unexpected error.

[Cito]

Thanks again for providing this PR. I agree that due to the changed behavior in Py 3.8 this should be simplified since aclose lost its significance as it cannot be called on a running generator anymore.

Unfortunately, I did not manage to cut the pre-release and get this merged in this weekend, so this needs to wait a few more days. But the current codebase is already up to date with GraphQl-js 17.0.0a2 with the defer/stream support.

If you have some time in between, maybe you can have a look at the mypy errors and the coverage issue (probably can just be silenced with a comment). Also, maybe you can make use of aclosing from the contextlib in map_async_iterable. Another suggestion is to structure and name the tests similar to mapAsyncIterable-test.ts. If some are not applicable, try to do the closest possible equivalent.

But if you don't have the time, I can also do it myself next weekend.

[kristjanvalur]

I'll have a look at errors/coverage. The reason I didn't use aclosing() is that this decorator requires that the iterator has an aclose() method. this is not a requirement for AsyncIterators in general, and subscriptions can be created using home-grown iterators rather than async generators. Hence the requirement to check for the presence of an aclose(), before calling it.

[kristjanvalur]

Ah, https://github.com/graphql/graphql-js/blob/main/src/execution/__tests__/mapAsyncIterable-test.ts so this is where the weird tests came from. A lot of these tests are designed to test behaviour which is simply guaranteed with Async Generators. I expect that the reason for all these tests in the .ts implementation is that the implementation needed to be much more complex and so a lot of tests were required to test expected iterator/generator semantics.

This is not necessary in Python, and even though we are trying to keep a "mirror" reference implementation of the Typescript reference, there are places, like AsyncGenerators, where we don't need to carry all that baggage over.

I deleted a lot of the python tests which were to try to test edge cases of the old implementation. Some of these were even weirdly incorrect, like trying to yield values after receiving a GeneratorExit.

For this reason, I don't think it makes sense to try to duplicate all of these tests, simply because the source tests may be trying to emulate behaviour which may be different in Typescript and in Python. But if there are any specific tests you feel that are missing, I'll be happy to add them.

[Cito]

This is now released in 3.3.0a3.

[kristjanvalur]

Btw, it is funny (to me) that you are changing the signature of subscribe() to be hybrid-async, when I am maintaining a library enabling authors to avoid the hybrid pattern and stay async: https://github.com/kristjanvalur/py-asynkit#coro_sync---running-coroutines-synchronously Sure this is just third party code, but I hope one day that we can avoid all this code duplication and helper method nonsense and just have a single implementation for libraries, maybe using a similar pattern.

[Cito]

Yes, that sounds counter-productive, but the explanation is that the goal of GraphQL-core is to be a very simple 1:1 port of GraphQL.js with no other dependencies, and this change of the signature simply reflects a similar change that was made in GraphQL.js. Staying very close to the original is currently the only way I can maintain the project and keep it up to date with upstream in my very limited spare time. But I agree there is much potential for optimization and harmonization.