python-trio / trio-websocket

WebSocket client and server implementation for Python Trio
MIT License
70 stars 25 forks source link

connection fails on URL without a path component #148

Closed goodboy closed 3 years ago

goodboy commented 3 years ago

Not sure who's guilty here (kraken's impl or this client) but downgrading to 0.8.1 resolves the problem. Their api has been in production for a while.

Test code:

import trio
import trio_websocket

async def open_ws():
    async with trio_websocket.open_websocket_url(
        'wss://ws.kraken.com',
    ) as ws:
        ...

trio.run(open_ws)

On 0.8.1 this yields no output but on 0.9.0 this errors with:

Traceback (most recent call last):
  File "tws_kraken_fail.py", line 10, in <module>
    trio.run(open_ws)
  File "/home/goodboy/repos/piker/3.8/lib/python3.8/site-packages/trio/_core/_run.py", line 1928, in run
    raise runner.main_task_outcome.error
  File "tws_kraken_fail.py", line 5, in open_ws
    async with trio_websocket.open_websocket_url(
  File "/home/goodboy/repos/piker/3.8/lib/python3.8/site-packages/async_generator/_util.py", line 34, in __aenter__
    return await self._agen.asend(None)
  File "/home/goodboy/repos/piker/3.8/lib/python3.8/site-packages/async_generator/_impl.py", line 366, in step
    return await ANextIter(self._it, start_fn, *args)
  File "/home/goodboy/repos/piker/3.8/lib/python3.8/site-packages/async_generator/_impl.py", line 202, in send
    return self._invoke(self._it.send, value)
  File "/home/goodboy/repos/piker/3.8/lib/python3.8/site-packages/async_generator/_impl.py", line 209, in _invoke
    result = fn(*args)
  File "/home/goodboy/repos/piker/3.8/lib/python3.8/site-packages/trio_websocket/_impl.py", line 124, in open_websocket
    raise DisconnectionTimeout from None
  File "/home/goodboy/repos/piker/3.8/lib/python3.8/site-packages/trio/_core/_run.py", line 815, in __aexit__
    raise combined_error_from_nursery
  File "/home/goodboy/repos/piker/3.8/lib/python3.8/site-packages/trio_websocket/_impl.py", line 1182, in _reader_task
    await self._send(self._initial_request)
  File "/home/goodboy/repos/piker/3.8/lib/python3.8/site-packages/trio_websocket/_impl.py", line 1238, in _send
    data = self._wsproto.send(event)
  File "/home/goodboy/repos/piker/3.8/lib/python3.8/site-packages/wsproto/__init__.py", line 35, in send
    data += self.handshake.send(event)
  File "/home/goodboy/repos/piker/3.8/lib/python3.8/site-packages/wsproto/handshake.py", line 88, in send
    data += self._initiate_connection(event)
  File "/home/goodboy/repos/piker/3.8/lib/python3.8/site-packages/wsproto/handshake.py", line 338, in _initiate_connection
    upgrade = h11.Request(
  File "/home/goodboy/repos/piker/3.8/lib/python3.8/site-packages/h11/_events.py", line 71, in __init__
    self._validate()
  File "/home/goodboy/repos/piker/3.8/lib/python3.8/site-packages/h11/_events.py", line 147, in _validate
    validate(request_target_re, self.target, "Illegal target characters")
  File "/home/goodboy/repos/piker/3.8/lib/python3.8/site-packages/h11/_util.py", line 108, in validate
    raise LocalProtocolError(msg)
h11._util.LocalProtocolError: Illegal target characters

Please feel free to adjust the issue title, I haven't dug into this at all other then quick looks through the debugger stack.

Underlying issue seems to be the h11._events.Request.target is b'' and it dies inside .validate()?

belm0 commented 3 years ago

I assume you're running wsproto 1.0?

Please try wsproto 0.14 to narrow things down.

goodboy commented 3 years ago

I don't think so actually. When I upgrade to 0.9.0 I get wsproto==0.14.1.

Code is in a muck atm will report back with downgrade to 0.14 shortly!

belm0 commented 3 years ago

Right, I meant 0.14.1. Does sound like a regression in trio-websockets then.

bluetech commented 3 years ago

This regressed in 0233e2176731a190a515c025c0762c19fbde7821.

It boils down to:

yarl.URL("wss://ws.kraken.com").path  # '/'

urllib.parse.urlsplit("wss://ws.kraken.com").path  # ''

and h11 doesn't like this empty path.

I'll need to look a bit to find the correct fix, but for now you can work around it by using wss://ws.kraken.com/ (note the trailing slash).

bluetech commented 3 years ago

RFC 3986 (URL spec) says a zero-length path is allowed, so the urlsplit result on itself is correct.

RFC 7230 (HTTP spec) says "If the target URI's path component is empty, the client MUST send "/" as the path within the origin-form of request-target.". So h11 is strictly correct there.

So I guess now the question is where should the path if path else '/' condition go: trio-websocket, wsproto, h11?

bluetech commented 3 years ago

Since h11 and wsproto both use the HTTP term target, and only trio-websocket use the term path, and handles URLs, I think it should go in trio-websocket. I'll send a PR.

andersea commented 3 years ago

There is a similar issue if you supply an empty resource in the trio_websocket.open_websocket and trio_websocket.connect_websocket calls.

goodboy commented 3 years ago

@bluetech amazing. thanks for the investigation and fix!

I assume minor release will include this soonish?

I'll need to look a bit to find the correct fix, but for now you can work around it by using wss://ws.kraken.com/ (note the trailing slash).

Will probably use this in meantime.

bluetech commented 3 years ago

@goodboy, @belm0 has already released 0.9.1!

@andersea These ones are not entirely clear-cut like the URL one, I mean I'm not sure I'd consider '' to be a "resource". And I also figure it was always the behavior...

goodboy commented 3 years ago

@bluetech indeed this is all solved for me thanks for the help :surfer: