Implement isConnected method

[bhartnett]

Having an isConnected method on the RpcClient will be useful for handling reconnection scenarios especially for websockets. At the moment if the websocket server goes offline then restarts the client is disconnected and not reconnected. Having an isConnected proc will allow checking and reconnecting in applications.

[bhartnett]

In this test when I try to close the socket client it gets stuck waiting and it looks like the socker server loop never exits/finishes for some reason.

[jangko]

it's a know bug of socket client

[bhartnett]

I didn't spend long looking at it. Do you have a suggestion/guidance on how I can fix it? If it isn't trivial, then I'll create an issue for it.

[jangko]

The problem is here I think, but I have not actually test it because we do not have production code using socket client or server.

nim-json-rpc/json_rpc/clients/socketclient.nim

Line 85 in be16a65

var value = await client.transport.readLine(defaultMaxRequestLength)

[bhartnett]

I've just pushed a fix which solves the problem in the tests. Can you review the change when you get a chance? I'm not sure if it might effect something else.

[jangko]

RpcHttpClient actually not using persistent connection like RpcWebsocketClient or RpcSocketClient. RpcHttpClient does not using keep-alive connection. Every request actually a new connection. I don't think we need to add isConnected to RpcHttpClient.

[bhartnett]

RpcHttpClient actually not using persistent connection like RpcWebsocketClient or RpcSocketClient. RpcHttpClient does not using keep-alive connection. Every request actually a new connection. I don't think we need to add isConnected to RpcHttpClient.

I see, in that case it will be unimplemented and throw an assertion Defect if called on the RpcClient type. That might create a problem. For example, I've created a type that uses RpcClient and it might be a RpcHttpClient or a RpcWebsocketClient and I'll like to check the connection if an error is thrown in these cases with having to right code that checks what type of instance it is.

Perhaps it would be better to simply return true for the RpcHttpClient? The RpcHttpClient does have a connect method already so perhaps isConnected in this context just means that it is ready to send requests.

[jangko]

Perhaps it would be better to simply return true for the RpcHttpClient?

I have no objection, although it's a bit misleading technically. But I think it's ok. isConnectedOrReadyToSend is too wordy.

[bhartnett]

Perhaps it would be better to simply return true for the RpcHttpClient?

I have no objection, although it's a bit misleading technically. But I think it's ok. isConnectedOrReadyToSend is too wordy.

Yes thats true but perhaps if we implement keep alive then it won't matter. In that case, I'll leave it for now.

[arnetheduck]

Having an isConnected proc will allow checking and reconnecting in applications.

this is based on the assumption that connectivity accurately can be established, which is usually not true - ie the normal case is that you discover if you're connected or not on the first read or write from the socket, so the right place to handle reconnections is usually in one of those two places: where you wait for new data to arrive and where you attempt to send stuff.

[bhartnett]

Having an isConnected proc will allow checking and reconnecting in applications.

this is based on the assumption that connectivity accurately can be established, which is usually not true - ie the normal case is that you discover if you're connected or not on the first read or write from the socket, so the right place to handle reconnections is usually in one of those two places: where you wait for new data to arrive and where you attempt to send stuff.

What would you suggest for this PR in that case? The overall goal for this PR was to have a way to decide whether to try reconnecting when using websockets and catching an error after issuing a json-rpc request. If the web socket connection is lost then a reconnect is required in order to continue sending data.

In the RpcWebSocketClient type the WSSession transport field gets set to nil when the connection is lost and as you said this is only discovered when sending or receiving. Currently the only way for me to handle this is to check if the wsClient.transport field is nil which doesn't seem like a very nice solution.

[arnetheduck]

What would you suggest for this PR in that case?

The "broad" way of handling this is to have the thing doing the requesting decide if it should retry based on the outcome of the request itself - the way sockets work is that many connection failures are detected when you try to use the socket, so this is usually the best place to "schedule" retries.

Wanting to check for isConnected usually indicates that there is some underlying architectural issue, for example that it is not clear who "owns" the responsibility to open and close the connection when a request is being made.

[bhartnett]

What would you suggest for this PR in that case?

The "broad" way of handling this is to have the thing doing the requesting decide if it should retry based on the outcome of the request itself - the way sockets work is that many connection failures are detected when you try to use the socket, so this is usually the best place to "schedule" retries.

Wanting to check for isConnected usually indicates that there is some underlying architectural issue, for example that it is not clear who "owns" the responsibility to open and close the connection when a request is being made.

It sounds like a better design for this might be to simply throw an exception at the time of doing a request which clearly indicates that a reconnect is required. The calling code can then catch and then handle appropriately. I'll close this PR for now and come up with a better solution.