Conversation

wuyuanyi135

No description provided.

…tasks cancel exception is not caught and the internal __aiter__ task will not be stopped.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for this PR which looks reasonable to me. If possible, improve the formatting and test coverage so that CI tests pass. Otherwise let me know, I can do the polishing then.

@wuyuanyi135

@Cito There might be a break change. Previously the exception of wait was not handled and the MapAsyncIterator will exit with uncaught exception. So, if an async task is using this code to acquire data and handle the task cancel event, this PR will disable this behavior as the code will run into the # After StopAsyncIteration section without exception. Do you think instead of StopAsyncIteration, it should raise a destructive exception like re-raise the CancelledError to maintain the behavior?

try:
    async for result in iterator:
        ...
    # After StopAsyncIteration
except BaseException as e:
    ...
    

@wuyuanyi135

Also the Iterator should be marked as is_closed in the except block.

@wuyuanyi135

@Cito I have fixed the code according to the guideline. My test code is quite shabby that requires some asyncio.sleep because it may otherwise not triggering the errors. I may need your help to normalize the techniques.

@Cito

Thank you for adding the test. I think using asyncio.sleep is ok. In principle, we could use something like aiofastforward or asyncio-time-travel to make it run faster, but I don't want to add yet another 3rd party pytest plugin.

Do you think instead of StopAsyncIteration, it should raise a destructive exception like re-raise the CancelledError to maintain the behavior?

In fact I also thought of re-raising CancelledError instead. Let's fix it that way first, it looks like the right/safe thing to do. Let me know if you want to change it or I shall do it.

@wuyuanyi135

@Cito I'm currently away from my computer. It would be nice if you can help change the code and tests. Thank you.

@Cito

Ok, I'll take care of this. Thanks for the contribution.

@CitoCito merged commit 62ddc6c into graphql-python:main May 2, 2021
Cito added a commit that referenced this pull request May 2, 2021
As discussed in #131, better than conversion to StopAsyncIteration.
@wuyuanyi135

Hi @Cito, can I ask when will the update release with this fix?

@Cito

I want to release 3.1.5 with this fix as soon as I caught up with all changes in graphql-js 15.4.0, probably end of this week.

Please check whether the current main branch works for you. I have already made the mentioned changes.

@wuyuanyi135

@Cito Thank you for the information

@Cito

Sorry, could not finish the release this week. May take one or two weeks longer.

@wuyuanyi135

@Cito Not at all! Thanks for the information!

@Cito

Found some time to publish the release today.

Sign up for free to join this conversation on . Already have an account? Sign in to comment
None yet
None yet

Successfully merging this pull request may close these issues.