Skip to content

[core] fix "read" not awaited warning in ContentDecodePolicy for async responses #21318

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
iscai-msft merged 4 commits into from
Oct 27, 2021
Merged

[core] fix "read" not awaited warning in ContentDecodePolicy for async responses #21318

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
4d2955b
only call read on sync responses in ContentDecodePolicy
iscai-msft Oct 18, 2021
b518af8
update changelog
iscai-msft Oct 18, 2021
b208403
allow to work in 2.7
iscai-msft Oct 26, 2021
65454a1
Merge branch 'main' of https://github.com/Azure/azure-sdk-for-python …
iscai-msft Oct 26, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions sdk/core/azure-core/CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@
### Bugs Fixed

- respect text encoding specified in argument (thanks to @ryohji for the contribution) #20796
- Fix "coroutine x.read() was never awaited" warning from `ContentDecodePolicy` #21318

### Other Changes

Expand Down
5 changes: 3 additions & 2 deletions sdk/core/azure-core/azure/core/pipeline/policies/_universal.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@
"""
from __future__ import absolute_import # we have a "requests" module that conflicts with "requests" on Py2.7
import json
import inspect
import logging
import os
import platform
Expand Down Expand Up @@ -617,10 +618,10 @@ def deserialize_from_http_generics(
mime_type = "application/json"

# Rely on transport implementation to give me "text()" decoded correctly
if hasattr(response, "read"):
if hasattr(response, "read") and not inspect.iscoroutinefunction(response.read):
# since users can call deserialize_from_http_generics by themselves
# we want to make sure our new responses are read before we try to
# deserialize
# deserialize. Only read sync responses since we're in a sync function
response.read()

@xiangyan99 xiangyan99 Oct 18, 2021

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Two questions:

  1. We have a few read() in ContentDecodePolicy, why we only need to change this?
  2. Why we read() in sync while not read() in async? Will this inconsistent behavior cause problems?

@iscai-msft iscai-msft Oct 19, 2021

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

  1. This is the only read on a response body, the other read is on data, not on response, so that is sync
  2. Since this is a sync method, we can only read for sync. Not reading async is consistent with the overall flow of async where we don't do any blocking calls / read in all of the information until necessary

@xiangyan99 xiangyan99 Oct 20, 2021

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Then should we have a AsyncContentDecodePolicy? (apologize if we have discussed this before)

return cls.deserialize_from_text(response.text(encoding), mime_type, response=response)

Expand Down