Thanks! See my comment about ErrorsCh. We'll need to add the same select there. Maybe add a func, which will wrap this.

Codecov Report

Merging #777 into develop will decrease coverage by 0.2%.
The diff coverage is 70%.

@@             Coverage Diff             @@
##           develop     #777      +/-   ##
===========================================
- Coverage    57.74%   57.54%   -0.21%     
===========================================
  Files           82       82              
  Lines         8440     8434       -6     
===========================================
- Hits          4874     4853      -21     
- Misses        3150     3160      +10     
- Partials       416      421       +5

@melekes well it was quick enough to do so the second commit does that, if you guys want to go that way (which would suit me, since I'd like to get at ID)

I converted ResultsCh into ResponsesCh. To surface the justification from an outdated diff:

Actually this reminds me of an issue with WSClient, which might make sense to do something about now. It really would be better if it just exposed a chan RPCResponse rather than filtering them into ResultsCh and ErrorsCh because:

1. It forces callers to maintain and synchronise two goroutines to read a single logical stream of messages.
2. It would be easy to accomplish the same as a caller by launching a goroutine that reads from a single chan RPCResponse
3. (And by far the most important) currently callers have no access to the request/response correlation ID RPCResponse.ID that is set by the client and is meant to allow the client to map the Responses.

There's actually only 13 usages of ResultsCh in the Tendermint codebase. Maybe now would be a good time to fix this issue and just provide a ResponseCh. I think I could be persuaded to do the work to port it over in the Tendermint codebase, but are there other users we need to worry about a breaking change with?

I like the unified responses channel, but it feels wrong to have so many quit channels. Can this not be simplified somehow to just use c.Quit?

Why yes, we can...

At least my additional channel is unnecessary if we can depend on BaseService closing its Quit channel as it currently does (probably for this reason) so once it is closed it will emit unlimited quit signals, so unlike what I had in mind where a single signal would need to be distributed to readRoutine and writeRoutine, instead they can both be signalled to quit from Quit.

I had a look to see if we could do without the readRoutineQuit channel, but it is legitimately a different signal to Quit since it is really a 'reset' signal clearing the way for a reconnect, whereas Quit signals permanent irrevocable stopping of the service. We could probably piggy back on some other state to unambiguously detect the reconnect attempt in writeRoutine and use that to shut it down (like maybe the reconnectAfter channel), but I think it is plainer the way it is.

Last commit removes my extra channel. Let me know if you want a squash.