Use Python 3.8 asyncio.Stream where possible

[JayH5]

Fixes #256 (although there is still one warning somewhere in websockets).

On Python < 3.8, this wraps the asyncio.StreamReader/asyncio.StreamWriter objects in a StreamCompat type with a similar interface to the new asyncio.Stream. On Python >= 3.8, asyncio.Stream is used instead.

Similarly, on Python < 3.8 loop.open_connection() is used, but on >=3.8 loop.connect() is used.

[JayH5]

await-ing on the drain call is not exactly the same as how things work in Python 3.8 when you await on Stream.write() it first tries a fast-path to flush data and then falls back to a full drain.

[florimondmanca]

Apparently drain() was deprecated in 3.8 too, and it is now recommended to await stream.write(). Should we switch to this latter API? This means we'd need to refactor AsyncioBackend.write() a bit, and potentially use a manual buffering mechanism for .write_no_block(). (There's an example of this in the trio backend: #276.)

[JayH5]

I was hoping this could tie into #341 quite nicely maybe?

[florimondmanca]

Yes, actually it does. :)

[sethmlarson]

Just dropping in to say that this is an awesome change, thank you so much! :)

[JayH5]

With the await, the test_start_tls_on_socket_stream test fails transiently with the error mentioned in this thread in about 1 out of 5-30 test runs (thanks, pytest-repeat) for me on Python 3.7.2, 3.7.4, and 3.8.0b4.

There is a possible fix (a custom exception handler set on the event loop) in the thread, but it's fairly complicated and I'm not sure how to integrate it nicely. This is no worse than we had so I think it can be fixed in a separate PR.

[sethmlarson]

Wow, that thread doesn't look good. 😨

[florimondmanca]

Is it OK to not await this .close() call, though (since we did await it before)? Will it close itself in the background in pure asyncio magic?

[JayH5]

@florimondmanca

since we did await it before

afaics, we don't/never have await-ed on the close/wait_closed?

We should probably await the close, though. I've looked at this a bit more and read the aiohttp thread a bit more thoroughly and...

• The SSL error occurs sporadically when await-ing on StreamWriter.wait_closed() (under the hood, Python 3.8 (for now) calls Stream.wait_closed() when you await Stream.close())
• Python 3.6 doesn't have wait_closed so this doesn't apply.
• This seems to only apply to OpenSSL 1.1.1+ since that is when this particular type of error was added.

The best I've come up with is something like:

async def close(self) -> None:
    try:
        await self.stream.close()
    except ssl.SSLError as e:  # pragma: no cover
        if e.reason == "KRB5_S_INIT":
            logger.debug("Ignoring asyncio SSL KRB5_S_INIT error on stream close")
            return

        raise

Thoughts?

I'm not sure exactly what state the socket is in when that error is raised 😕. Once we try to close and get that error there doesn't seem to be anything we can do without just getting that error again.

[florimondmanca]

Thanks, all these details are very helpful. :)

Since the 3.8 docs it’s only possible to await on .close() (which I find kind of weird, having in mind all those « coroutine was never awaited » exceptions that usually causes), I’m okay with keeping it as it is currently. As you said, we actually never waited for the socket to close before (and if we did, we’d have probably only encountered this issue earlier?).

[JayH5]

One (not very good) reason to include this try/except is that without it I don't think this change has 100% test coverage since wait_closed is never called.

[JayH5]

I was hoping this could tie into #341 quite nicely maybe?

[sethmlarson]

If you're good with this @florimondmanca we can merge?

[florimondmanca]

Note to myself that I’ll have to deal with the changes in #341 once this get merged. :)

[tomchristie]

Crackin work, thank you! ✨

To the team in general - can we peddle just a wee bit slower on getting this one in?

We need to unify StreamReader and StreamWriter into a single Stream interface in any case, and I'm a bit uncomfortable with some basics such as having such a deeply nested file layout.

I'd quite like to approach this, by tackling the StreamReader+StreamWriter -> a single interface first, and then looking at how 3.8 support changes that.

Also in cases where we need to have version-specific branches I typically try to isolate those strictly all into a compat.py module, that can then be excluded from coverage reporting. I think we may well want to have a bit of a review of if we can/should be doing the same with httpx.

[florimondmanca]

@tomchristie The reader/writer pair is very much specific to asyncio — our stream interface which concurrency backends manipulate is already a single interface. As a matter of fact, I don’t think this PR changes any of that. Instead, it basically reimplements asyncio.Stream for < 3.8 so that we can use that to implement the existing Stream interface in the asyncio backend. So is there anything I’m missing about existing reader/writer interfaces?

[tomchristie]

Ah okay makes sense - I’m getting a bit back up to speed and probably need to take another look over.

[sethmlarson]

@JayH5 This looks like it needs some conflict resolution with master before it can land. Mind taking a look? :)

[JayH5]

git didn't have any problem merging master. I guess GitHub must use a simpler merging algorithm 🤷‍♂

[sethmlarson]

Thanks for much for this contribution @JayH5! 🙌 Hope to see more work in our complex async machinery from you. :)

[tomchristie]

Not a big problem, but from my POV we've moved too fast on merging this. 🏎🌬
I'd really like us to slow down on substantial changes like this. 😇

(Even without the change in Python 3.8 I was expecting to take a fuller review onto it, first.)

Given #256 (comment) should we raise an issue to just plain revert this, or something else?