Add doHandleWebsocket function #946
Conversation
This allows establishing a websocket connection directly in an HTTPServerRequestDelegate, which is useful if one needs to make some checks on the request (and possibly send an http response with an error code) before opening the websocket. Signed-off-by: Luca Niccoli <l.niccoli@awtech.it>
| res.headers["Connection"] = "Upgrade"; | ||
| ConnectionStream conn = res.switchProtocol("websocket"); | ||
|
|
||
| scope socket = new WebSocket(conn, req); |
There was a problem hiding this comment.
How does this compile without warning ?
There was a problem hiding this comment.
If you refer to scope it was never actually deprecated and still works as it did in D1 days.
There was a problem hiding this comment.
Any reason behind that ? In Vibe.d's case, would Scoped be a good (better?) replacement ?
There was a problem hiding this comment.
auto socket = scoped!WebSocket(conn, req); should work unless WebSocket does something nasty.
There was a problem hiding this comment.
both are inherently unsafe though as on_handshake does not guarantee it won't escape the reference
There was a problem hiding this comment.
both are inherently unsafe though as
on_handshakedoes not guarantee it won't escape the reference
Why not? on_handshake is defined as void delegate(scope WebSocket) so in theory it should not be able to escape the reference to the WebSocket. Are scope parameters not working yet?
There was a problem hiding this comment.
Anyway, WebSocket does not even have a destructor defined. Shouldn't we just drop the scope?
There was a problem hiding this comment.
Are scope parameters not working yet?
Not even slightly so.
| */ | ||
| HTTPServerRequestDelegate handleWebSockets(WebSocketHandshakeDelegate on_handshake) | ||
| void doHandleWebsocket(WebSocketHandshakeDelegate on_handshake, HTTPServerRequest req, HTTPServerResponse res) |
There was a problem hiding this comment.
The on_handshake parameter could be scope WebSocketHandshakeDelegate here to avoid creating heap delegates at the call site.
|
Thanks, this change makes a lot of sense. I'm just still a little unsure about the best name. Usually I'd simply use an additional overload of |
Do not declare the WebSocket a scoped variable, it's dangerous and does not seem to be useful anyway, since WebSocket does not declare a destructor. Do not simulate socket closing if the connection delegate throws an Error, the recommended action is terminating the application anyway. Signed-off-by: Luca Niccoli <l.niccoli@awtech.it>
lultimouomo
force-pushed
the
doHandleWebsocket
branch
from
93a817b
to
38ef271
Compare
|
|
Add doHandleWebsocket function
This allows establishing a websocket connection directly in an
HTTPServerRequestDelegate, which is useful if one needs to make some
checks on the request (and possibly send an http response with an error
code) before opening the websocket.
Signed-off-by: Luca Niccoli l.niccoli@awtech.it