Fix auto reconnect for RPC WebSocket

[nguyer]

In the case of a reconnect the resChl was nil so sending nil to it ended up blocking, causing the receiver to not receive any further messages.

[peterbroadhurst]

Digging into this... it needs some thinking to understand why it's nil - and I've not spotted that in code comments or the PR comment.

[nguyer]

Digging into this... it needs some thinking to understand why it's nil - and I've not spotted that in code comments or the PR comment.

Agreed. I'm not sure why it's nil here either. Though the if statement I added seemed to be consistent with the existing comment in the code if someone is waiting...

[peterbroadhurst]

So I see here that we set it into nil, but that's when we've called handleSubscriptionConfirm - so that scenario would be a double dispatch:

firefly-signer/pkg/rpcbackend/wsbackend.go

Line 438 in 356a095

inflightSub.newSubResponse = nil // we only dispatch once (it's only new once, on reconnect it's old and there's nobody to tell if we fail)

@nguyer - I think to review this, I need a bit more information.
Do you have thread stacks or other things that lead to you an explanation of the reconnect scenario, and how this could be nil - and further more how that leads to a hang?

[peterbroadhurst]

More likely from just code inspection to me would seem like:

• Subscribe() gos into case rpcErr := <-newSubResponse:
• The context closed
• Subscribe() fails
• The hang happens, because there wan't a "slot" created on the make() of the channel

... if that were the bug, then the fix would be on this line:

firefly-signer/pkg/rpcbackend/wsbackend.go

Line 223 in 356a095

newSubResponse: make(chan *RPCError),

to change it to make(chan *RPCError, 1),

[nguyer]

firefly-signer/pkg/rpcbackend/wsbackend.go

Line 223 in 356a095

newSubResponse: make(chan *RPCError),

to change it to make(chan *RPCError, 1),

I just did a quick test of this and it still resulted in inflightSub.newSubResponse being nil when it came into handleSubscriptionConfirm.

I will keep digging.

[nguyer]

After digging into this more, it looks like this is the expected behavior that we only send the notification when the subscription is new, and not on reconnects:

firefly-signer/pkg/rpcbackend/wsbackend.go

Lines 228 to 230 in 356a095

	// need to return newSubResponse because it's a use-once thing (not on reconnect)
	// and will be nilled out (under lock) when the first creation response comes in
	return s, s.newSubResponse

[peterbroadhurst]

Thanks @nguyer, and for pointing me at:

firefly-signer/pkg/rpcbackend/wsbackend.go

Lines 228 to 230 in 356a095

	// need to return newSubResponse because it's a use-once thing (not on reconnect)
	// and will be nilled out (under lock) when the first creation response comes in
	return s, s.newSubResponse

I understand now the nil case I think, but I actually think we also need the , 1 change for another potential edge case I mention around the context.

So let's include both changes.