Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use ReconnectingSocket in Tendermint and Ethereum #1093

Merged
merged 8 commits into from
Jun 26, 2019

Conversation

willclarktech
Copy link
Contributor

Closes #229

Opening a draft PR to discuss API. In particular, the EthereumConnection tests call socket.disconnect() but currently if you do this on ReconnectingSocket before the connection has been successfully established you get an error. Should this be changed or should the higher-level WS clients expose a stream of connection statuses?

@webmaster128
Copy link
Contributor

Should this be changed or should the higher-level WS clients expose a stream of connection statuses?

I think calling

      const socket = new ReconnectingSocket(socketServerUrl);
      socket.connect();
      socket.disconnect();

should be valid, especially since it is hard for an application to know when the connection was established. I think this.unconnected should be changed to just guard against wrong order of calls, like double connect() or disconnect() without connect(), which indicate programming errors.

There is a test "can disconnect before waiting for open" in packages/iov-socket/src/socketwrapper.spec.ts and I think a similar test should pass for ReconnectingSocket as well.

@willclarktech willclarktech force-pushed the 229-reconnect-tendermint-ethereum branch from 4860e31 to 01536c8 Compare June 26, 2019 08:53
@willclarktech willclarktech force-pushed the 229-reconnect-tendermint-ethereum branch from c5f9d15 to ff9a00d Compare June 26, 2019 10:29
@willclarktech willclarktech marked this pull request as ready for review June 26, 2019 10:30
@willclarktech willclarktech force-pushed the 229-reconnect-tendermint-ethereum branch 2 times, most recently from d0309f0 to 6b88f4b Compare June 26, 2019 13:45

return response;
}

public async socketSend(request: JsonRpcRequest, ignoreNetworkError: boolean = false): Promise<void> {
await this.socket.connected;
public async socketSend(request: JsonRpcRequest, ignoreNetworkError: boolean | undefined): Promise<void> {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why this signature change? This requires the caller to set ignoreNetworkError to something. If we want that, we can remove the undefined option.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Whoops, originally removed the parameter entirely then added it back in with the compiler's suggestion.

@@ -200,7 +195,15 @@ export class WebsocketClient implements RpcStreamingClient {
* so this should be required for testing purposes only.
*/
public async connected(): Promise<void> {
return this.socket.connected;
return new Promise(resolve => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

await this.socket.connectionStatus.waitFor(ConnectionStatus.Connected)

This does what you implement below and also resolves when the value is ConnectionStatus.Connected already.

@willclarktech willclarktech force-pushed the 229-reconnect-tendermint-ethereum branch from 6b88f4b to 874829a Compare June 26, 2019 14:28
@willclarktech willclarktech merged commit 5127370 into master Jun 26, 2019
@willclarktech willclarktech deleted the 229-reconnect-tendermint-ethereum branch June 26, 2019 14:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Implement retries into WS and HTTP RPC
2 participants