refactor hierarchy of HttpPolicy & AsyncHttpPolicy

[xiangyan99]

No description provided.

[xiangyan99]

#5797

[chlowell]

Sure, some AsyncHTTPPolicy implementations happen to subclass HTTPPolicy (seems to me that's a problem, if perhaps a separate one). But this implies next.send() may not return a coroutine. In addition to that being misleading, I expect mypy to consider await self.next.send() an error given this type.

[xiangyan99]

I don't think we require next to be an AsyncHTTPPolicy.

It is valid that we have an aync_retry_policy w/ a header_policy/user_agent_policy.

Which means, yes, next.send() may not return a coroutine.

[chlowell]

HeaderPolicy and UserAgentPolicy are both SansIOHTTPPolicy. AsyncPipeline wraps them in an async runner. Do we support or expect HTTPPolicy or HttpTransport in AsyncPipeline?

[lmazuel]

I would focus the PR on what it's trying to solve: the retry policy problem, and not this one. I'm not sure I'm ready to be opiniated here as a side effect of a "retry policy" fix. As @chlowell implies, the choice here is more subtle that it seems and it would requires its own PR (I know, I'm freak about atomic PRs, but I assume it :p)

[xiangyan99]

The problem here is we have

self.next = None # type: Union[HTTPPolicy, HttpTransport]

In HTTPPolicy

but

self.next = None # type: Union[AsyncHTTPPolicy, AsyncHttpTransport]

in AsyncHTTPPolicy

If we make AsyncHTTPPolicy a subclass of HTTPPolicy, mypy is not happy.

@lmazuel

[lmazuel]

Well, what about you leave Any for this PR and we talk about that outside of this PR scope? :)

Create a new issue if you want that you don't like the way "next" is typed, I'm fine with that, but that should not impact this PR.

[xiangyan99]

OK. I separate it into two PRs. This one will be focusing on cleaning up the hierarchy of HttpPolicy & AsyncHttpPolicy (the other PR depends on this change).

[chlowell]

In actual usage AsyncPipeline ensures self.next is not None, so I think it's reasonable for this not to be Optional. (It matters because subclasses will get mypy errors on self.next.send() if this is Optional.)

[xiangyan99]

But we set it to None here, right?

If I set it non-nullable, mypy complains here.

[chlowell]

Yes, but it won't be None when used by AsyncPipeline. Typing it as non-Optional saves subclasses from doing something clumsy purely to satisfy the type checker (for example) that doesn't reflect real usage. Perhaps the question is, Do we expect AsyncHTTPPolicy to be used without AsyncPipeline? If yes, the type should be Optional because subclasses can't assume someone else assigned a non-None value. But I think the answer is "no", and incidentally that's evidently the answer for HTTPPolicy.

[xiangyan99]

To be honest, I don't know why it does not complain with HTTPPolicy while we set it to None but declare it is not optional. (it seems to me not a correct behavior).

If we want to declare self.next is not optional, we have to give it a value here. Either AsyncHTTPPolicy or AsyncHttpTransport.

Given this is the AsyncHTTPPolicy class and Python does not support pointer. self.next = AsyncHTTPPolicy() will cause dead loop.

AsyncHttpTransport is an abstract class. Do you think self.next = AiohttpTransport() is a good idea?

One concern is aiohttp is optional (not installed by default). So it is possible that user wants to use AsyncHTTPPolicy but w/o aiohttp installed.

[chlowell]

Seems this is something I don't understand either. There's no error here:

class A:
    pass

class B:
    def __init__(self):
        self.thing = None  # type: A

reveal_type(B().thing)  # Revealed type is A

Assigning self.next an instance of something seems excessive to me as a means to override mypy's judgment. Using cast is ugly but its purpose is clear, it accomplishes the same thing, and has no runtime implications:

self.next = cast(Union[AsyncHTTPPolicy, AsyncHttpTransport], None)

[xiangyan99]

/azp run python - storage - ci

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).

[xiangyan99]

/azp run python - core - ci

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).