Skip to content
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.

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

Allow more control over handshake (#4) #62

Merged
merged 1 commit into from
Oct 23, 2018
Merged

Conversation

mehaase
Copy link
Contributor

@mehaase mehaase commented Oct 19, 2018

First attempt at this. Server handlers now receive a WebSocketRequest
instead of a WebSocketConnection. The request object provides the
client's handshake parameters and allows the server to finalize
handshake parameters. The server calls the requests accept()
method to finish the handshake and obtain a connection object.

This first version exposes the request URL, headers, and subprotocols
to the server. The server is allowed to select a subprotocol. In future
versions (given support in wsproto), we can expand the request object
to customize headers and choose extensions.

In addition, the tests are updated: one new test to cover the
selection of a subprotocol, plus updates to existing tests to handle
the new request->connection step.

Finally, the README and example server are updated to show the new
request->connection step.

First attempt at this. Server handlers now receive a WebSocketRequest
instead of a WebSocketConnection. The request object provides the
client's handshake parameters and allows the server to finalize
handshake parameters. The server calls the requests ``accept()``
method to finish the handshake and obtain a connection object.

This first version exposes the request URL, headers, and subprotocols
to the server. The server is allowed to select a subprotocol. In future
versions (given support in wsproto), we can expand the request object
to customize headers and choose extensions.

In addition, the tests are updated: one new test to cover the
selection of a subprotocol, plus updates to existing tests to handle
the new request->connection step.

Finally, the README and example server are updated to show the new
request->connection step.
@mehaase mehaase requested review from belm0 and njsmith October 19, 2018 02:12
@coveralls
Copy link

coveralls commented Oct 19, 2018

Pull Request Test Coverage Report for Build 43

  • 58 of 61 (95.08%) changed or added relevant lines in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+1.5%) to 81.405%

Changes Missing Coverage Covered Lines Changed/Added Lines %
trio_websocket/init.py 58 61 95.08%
Totals Coverage Status
Change from base Build 41: 1.5%
Covered Lines: 394
Relevant Lines: 484

💛 - Coveralls

@njsmith
Copy link
Member

njsmith commented Oct 19, 2018

We might want to make it so handlers have to await something to read the headers, etc. Consider a malicious client that connects to our server, and then sits there doing nothing. We want to be able to time them out eventually. But this means that we either need to provide some ad hoc mechanism to configure timeouts inside trio-websocket, or else, we need to start the user's server handler before we get all the headers, and do the header reading inside the server handler, so the user can impose their own timeout logic if they want.

@mehaase
Copy link
Contributor Author

mehaase commented Oct 23, 2018

I've been wondering about how to support timeouts. It seems like Trio leans towards, "let the caller specify the timeout". This feels like it will be tricky because the library has hidden certain parts of the protocol from the caller. For example:

async with open_websocket_url('ws://my.example'):
  msg = await queue.get()
  await ws.send_message(msg)

This little snippet hides two important details:

  1. Waits for the open handshake to finish before entering the block.
  2. Waits for the close handshake to finish before exiting the block.

Both of these operations could timeout, especially if the peer is malicious and leaves the handshake half-finished. But where can the caller place timeouts if they want to timeout the websocket handshakes but not waiting for an item from their internal queue?

This example makes me think that there should be some timeouts inside the library for things like handshakes. The library should allow timeouts to be disabled in case the caller wants to handle it themselves. Thoughts?

@belm0
Copy link
Member

belm0 commented Oct 23, 2018

something like:

with trio.move_on_after(CONNECTION_TIMEOUT) as cancel_scope:
    async with open_websocket_url('ws://my.example'):
        cancel_scope.shield = True
        msg = await queue.get()
        cancel_scope.shield = False
        cancel_scope.deadline = trio.current_time() + REPLY_TIMEOUT
        await ws.send_message(msg)

@njsmith
Copy link
Member

njsmith commented Oct 23, 2018

Shielding isn't what you want here... setting shield = True protects you from outside cancel scopes, but the scope you set it on still applies. So it's exactly the opposite of what you want: it prevents being cancelled from outside, but doesn't disable the timeout.

If you want to disable the cancel scope's deadline, just do cancel_scope.deadline = inf.

It's an interesting point that Trio doesn't have a convenient way to apply a timeout just to entering or exiting a context manager. I guess we could make a context manager wrapper that does this:

async with limit_entry_exit(open_websocket_url(...), entry_timeout=..., exit_timeout=...):
    ...

I don't know whether this will be a problem in practice? I'm curious to find out, but I don't see any way to do that except wait and see whether it frustrates people...

@mehaase
Copy link
Contributor Author

mehaase commented Oct 23, 2018

I'm moving the timeout discussion over to #64 so that I can merge this PR. The timeout issues go much deeper than what's been raised here, so I'll tackle them separately.

@mehaase mehaase merged commit ed3b2bc into master Oct 23, 2018
@mehaase mehaase deleted the improve_handshake branch October 25, 2018 13:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants