openresty / lua-resty-websocket

WebSocket support for the ngx_lua module (and OpenResty)
508 stars 106 forks source link

feat(client) enhancements to connect() #69

Closed flrgh closed 2 years ago

flrgh commented 2 years ago

Very WIP--mostly creating this as a place to have some discussion.

overriding some values in the handshake request

I'm not sure if overriding Sec-WebSocket-Key is necessary or even a good idea, but being able to do so gives us the ability to make proxying code more transparent as we please.

saving the upstream handshake response

This is something I didn't think of during the initial arch/design discussions but is necessary IMO, because without it we can't proxy the upstream's response headers downstream, which could be a blocker for some users when compared to proxy_pass.

The response-handling in the current code is very spartan:

https://github.com/openresty/lua-resty-websocket/blob/34f6b41e538151e7a5f013ff9709c1708a9958fa/lib/resty/websocket/client.lua#L202-L216

I'm not sure how much we should expand upon this within lua-resty-websocket itself. Thus far I've gone the route of minimal change and introduced a keep_response connect flag which prompts client:connect() to stash the raw response string, allowing the caller to read and parse it as desired (now that I write this, maybe it should be returned instead so that it can be gc-ed without having to resort to client.response = nil, which feels a little dirty).

The other side of the coin is "well, there are some FIXME's in here for more response validation, so maybe some of the response parsing should happen within client:connect() in order to address those things". Not sure where to draw the line here on caller vs library responsibility for the chore of response-parsing.

flrgh commented 2 years ago

Oops, I fat-fingered things. This is not ready for review in OpenResty land yet :) closing