search

sourcegraph / jsonrpc2

Package jsonrpc2 provides a client and server implementation of JSON-RPC 2.0 (http://www.jsonrpc.org/specification)
MIT License
190 stars 62 forks source link

No websocket.PingMessage emitter in jsonrpc2/websocket #40

Closed tomsun closed 3 years ago

tomsun commented 3 years ago

This behavior: https://github.com/gorilla/websocket/blob/58729a2165ebb9f1d023226076f660139c2e2e0c/examples/chat/client.go#L116 https://github.com/gorilla/websocket/blob/5ed622c449da6d44c3c8329331ff47a9e5844f71/examples/command/main.go#L87

Should it be implemented by jsonrpc2/websocket or by the user?

keegancsmith commented 3 years ago

Control messages of websocket shouldn't concern jsonrpc2. However, I took a look at our current implementation at jsonrpc2/websocket and don't provide a way for the user to safely send control messages. The code in jsonrpc2/websocket though is very minimal. I'd recommend vendoring it in and modifying it to send ping messages. https://github.com/sourcegraph/jsonrpc2/blob/master/websocket/stream.go

tomsun commented 3 years ago

@keegancsmith thanks for the suggestions! yep not the end of the world to fork that file: not the core of the problem I wanted to outsource to sourcegraph/jsonrpc2.

Seems one should be able to concurrently ping via WriteControl() though, so maybe it's not a major issue most of the time. https://github.com/gorilla/websocket/issues/652#issuecomment-730443492