Subscriptions issues

[0cv]

I'm currently having two issues around subscriptions.

1- It looks like after some inactivity, such as in the SubscribeUser* methods which may more often than not, not returning anything during a while, the listener times out after just a couple of minutes - also any network issue would result in the same. I'm using the basic example provided and I'd expect that following part produces at least an error but it doesn't:

         go func() {
		err := <-errs
		stop <- true
		log.Fatalf("RPC error: %s", err)
	}()

Did I miss something? Is there somewhere some methods which would listen for errors, reconnect automatically (or not), etc. ?

2- I'm also trying to figure out how to close a subscription specifically, and not all of them. *deribit.Exchange, or the stop variable returned during the initialization seem to close all of them

[adampointer]

Hi.

I think we have reconnect behaviour but maybe it does not behave the way you want it to. I apologise, this is not documented well. I have not had so much time to dedicate to this project recently. The existing method is the onDisconnect property.

See #10

Admittedly at first glance your method is batteries included. I didn't find that I had to reauthenticate after a disconnect. Maybe that is a different failure scenario we need to handle.

Yes there is a way to close an individual sub. Try using this..

exchange.Client().GetPublicUnsubscribe
exchange.Client()GetPrivateUnsubscribe

You can give it a slice of channel names.

https://github.com/adampointer/go-deribit/blob/master/v3/client/operations/get_public_unsubscribe_parameters.go

I am not at my computer until tomorrow. I will review your code then. Thanks for your contribution!

[0cv]

I had to re-authenticate, as otherwise I was getting an error from the server when trying to resubscribe (unauthorized). And if I didn't resubscribe, the subscription just didn't work. So my solution seems to work, but maybe that yours is just easier / more elegant. I will let you know, thank you for the heads up!

[adampointer]

Cool. I think using some of your logic as a default reconnect function but allowing users to use whatever they want as long as it fits the interface seems like a good idiomatic solution. As I said I will take a proper look when I am at my computer again in the morning.

[0cv]

So, this OnDisconnect method is indeed just here so that one has to implement the re-connection strategy.

And well, this can be complicated in case of subscriptions because the subscription array is private .subscriptions so no way to rebind the streams outside the package easily... My PR takes care of this as far as I could see. Another problem of the OnDisconnect is that the callback was called like 20+ second after the connection was lost, while this PR, built into the hearbeat, has a downtime of max 10s.

Maybe, one would not want to reconnect automatically (not sure why), or implement another strategy (fair enough), but then this shall be straight forward to re-connect automatically. At this time it's not the case. If we want to keep the OnDisconnect property, we would have to implement *deribit.Reconnect() where the Reconnect function is doing what we have in the PR. Also could we make it faster than 20+ seconds? I couldn't figure out why it's taking longer than the hearbeat period.

[adampointer]

I suspect that the we are hitting the following code on the first connection fail (10s) and wait for the second fail (20s) to actually reconnect.

...
if err := e.conn.ReadJSON(&raw); err != nil {
	if isTemporary(err) {
		continue
	}
	e.mutex.Lock()
...

Perhaps we try removing that isTemporary check. Reconnection is relatively inexpensive so I think we should attempt a reconnection even if it is a temporary transport error.

I like your reconnect method. Let's split it out into a separate method as you say and change the code in exchange.read so that we call Reconnect if nil e.g.

if f := e.OnDisconnect; f != nil { 
    f(e)
} else {
    e.Reconnect()    
}

I think the advantage of keeping this code in read rather than in the heartbeat is that we handle a disconnect on every message not just on a heartbeat so it potentially more responsive.

Finally we need to address the use of log within the code. This divides the go community and there is no definitive right and wrong here. But I do not like logging within a library as a rule. However it is useful in some cases and this is one of those cases. However some people (like me) like to use their own logging package like logrus in their main application and like all logging to use that one thing. As there is no standard log interfaces, my preferred approach here is to expose the io.Writer and allow users to override it if they want.