Response not passed to WSServerHandshakeError

[jeduden]

Long story short

As requested in comment on issue 701 opening a new issue

WSServerHandshakeError does not provide the actual HTTP Response body.
This means any error handling code, logging code cannot provide the error information that the server provides in the response body with the normal Client object.

In order to work around this issue we use a client like this to get access to the response:

class LoggingClientSession(aiohttp.ClientSession):
      @asyncio.coroutine
      def _request(self, method, url, **kwargs):
          self.last_response = yield from super()._request(method, url, **kwargs)
          return self.last_response

Expected behaviour

Provide the complete response object as a property in the error. This way any change to the interface of the standard response is propagated directly without additional code changes.

Actual behaviour

WSServerHandshakeError only forwards a subset of the response. See this code: https://github.com/aio-libs/aiohttp/blob/master/aiohttp/client.py#L434

Steps to reproduce

Connect to a websocket server that responds to an upgrade request with an error response with body.

Your environment

not relevant.

[asvetlov]

The problem is the response should be closed on failed websocket handshake.
It means you have no HTTP BODY.
Well, partial body might be available but fetching the full body raises a security issue: malware might generate handshake failure with endless body for example.
The best we could do is adding .partial_body attribute to ClientResponseError, WSServerHandshakeError inherits from it.

The change might make sense but it is a low priority issue from my perspective.
@fafhrd91 what do you think?

[fafhrd91]

exposing response is not good idea because of close requirements. or we can add WSServerHandshakeError.__del__ and close response in __del__
I don't have strong opinion on this topic.

[asvetlov]

I definitely don't like to expose the whole response object.
As an option we could just store a part of already received BODY (most likely at least very first bytes are present in aiohttp buffers already).
But I believe the partial body content is not very useful: it contains unpredictable amount ob body bytes.
If server answers gzipped content -- the partial body is a non-decodable garbage most likely.

After careful thinking I suggest closing the issue with wontfix label.

@jeduden please provide the real use case why do you need a response body on websocket upgrade procedure.

[fafhrd91]

I think partial body is fine for debugging purpose.

[asvetlov]

Well, a volunteer is needed for the task.

[jeduden]

@asvetlov a bit more context. The end-point providing the websocket is guarded by an authorization layer, which can stop the handshake before the ws protocol handlers are invoked. It returns different errors messages for the various authorization errors.

In our e2e tests which use the aiohttp client to connect to the server we need to expose these errors to properly log, report and potentially check them. Developers otherwise have a hard time analyzing the error situation. And include hacks like the one above.

Regarding partial response:
For proper reporting we would need the complete error.

It is quite normal for an http error to return a response body. ( sometimes json to offer structured error data) . I dont see why websockets on protocol level are a special case during the handshake phase.

Thinking a bit : the method Ws_connect is a bit different in its interface than its counter parts (get, post) : it doesnt return the actual response and doesnt offer raise_for_status.

The abstraction it provides is a bit too much for the error reporting case.
I think there two possible solutions to remove the mismatch:

1. An optional call back to let the caller process the response fully in the error case
2. New ws_connect variant. Return a response object. And that offers a method to access the websocket interface.

[asvetlov]

Changing return type for ws_connect is not an option: this is backward incompatible change.
Optional callback doesn't sound perfectly but it is less harm.
Or we could introduce a new ws_connect2 method with desired behavior, which is not clean for me yet. On one hand I'd like to give full control on processing failures, on other don't want to expose websocket upgrade response if it's passed successfully.

[jeduden]

@asvetlov agreed a difficult call to make. would you accept a pr with the addition of a call back?

i would suggest that the callback defines a factory-method for the exception to be thrown.

Would this be acceptable semantics ?