Fix WASM-client disconnection error

[ViktorTigerstrom]

In this PR, we add a fix that solves the error that the wasm-client freezes when attempting to close the sockets when disconnecting.

I've also added a commit to add the gbn subsystem to the wasm-client's logger. This simplifies debugging errors quite a lot. Let me know if you don't think that commit is worth including though :)

[ViktorTigerstrom]

Updated the PR with adding a callback that's executed once the disconnection has completed, so that the frontend only updates once we're fully disconnected.

I noticed that for checking wether we are connected or not, we instead use a ticker that checks the IsConnected func every 200ms to determine wether the connection setup has completed or not. If you'd like me to do the same for the disconnected version, I could of course do so, so let me know if I should change this :)!

[guggero]

Very nice, LGTM 🎉

[jamaljsr]

tACK 🚀

I've confirmed the disconnect issue is now fixed.

My only request would be to have some way to disable the additional logs that are now being outputted. It logs 2 lines every second which adds a lot of noise that isn't particularly useful info for us app devs. It makes it harder to read through our own logs. Could a config param be used to disable this?

[ViktorTigerstrom]

Thanks a lot for the reviews @guggero & @jamaljsr!

@jamaljsr, just wanted to check, is setting the debuglevel to info when you start the wasm-client an ok way for you app-devs to get rid of the extra gbn subsystem logs? I.e. with the example-server the log level is set to trace which is why the extra log lines are shown. https://github.com/lightninglabs/lightning-node-connect/blob/d8c9f92056e2b5a0c908514b2159d01738f7afb7/example/index.html#L54C14-L54C32

But in a real setting when using the wasm-client, I'd imagine that the debuglevel would be set to info, which removes the log extra lines you're referring to. Or does setting the debuglevel to info remove too much useful information for you?

[jamaljsr]

But in a real setting when using the wasm-client, I'd imagine that the debuglevel would be set to info, which removes the log extra lines you're referring to. Or does setting the debuglevel to info remove too much useful information for you?

Ah yes, great suggestion about tweaking the debuglevel param. While setting it to info wouldn't be ideal because it produces no output at all, I was able to fiddle with it and find the optimal level of logging with debuglevel=debug,GOBN=info,GRPC=info. This is even better than what we had previously as it was a bit too verbose before. This is perfect, thanks! 🙌

Updated the PR with adding a callback that's executed once the disconnection has completed, so that the frontend only updates once we're fully disconnected.

I prefer the callback approach over polling, so it looks good to me. My only nitpick would be to rename disconnected to onDisconnect

[ViktorTigerstrom]

Ah yes, great suggestion about tweaking the debuglevel param. While setting it to info wouldn't be ideal because it produces no output at all, I was able to fiddle with it and find the optimal level of logging with debuglevel=debug,GOBN=info,GRPC=info. This is even better than what we had previously as it was a bit too verbose before. This is perfect, thanks! 🙌

Awesome :rocket!

I prefer the callback approach over polling, so it looks good to me. My only nitpick would be to rename disconnected to onDisconnect

Great ok! I've renamed the callback to onDisconnect with the latest push. I saw that we also define some callbacks as config options instead:

lightning-node-connect/example/index.html

Lines 56 to 58 in d8c9f92

	'--onlocalprivcreate=' + namespace + '.onLocalKeyCreate',
	'--onremotekeyreceive=' + namespace + '.onRemoteKeyReceive',
	'--onauthdata=' + namespace + '.onAuthData',

If you'd like me to do the same for the onDisconnect callback let me know, but I found this approach I've implemented in the PR a bit cleaner.

[jamaljsr]

tACK LGTM 🚀

If you'd like me to do the same for the onDisconnect callback let me know, but I found this approach I've implemented in the PR a bit cleaner.

I like this approach as well.

[ellemouton]

is it maybe worth using a wait group just to ensure that things do actually complete?

[ViktorTigerstrom]

Thanks for the review and suggestion @ellemouton!

Unfortunately I did try that before, but using a WaitGroup and then Waiting for the WaitGroup at the end of the Disconnect function makes the same problem that existed prior to the fix occur. The main thread then blocks on the Wait and for some reason it never picks up that the Websocket is now Hard closed by the new updates to the grpc dependency (introduced in v0.18.1). I think this is because this is in a JavaScript environment, and the main thread therefore never picks up any changes to the Javascript objects as it's blocking. So the WaitGroup Wait would never complete.

I therefore chose the alternative to signal the completion by a callback instead, as that unblocks the main thread, and makes it pick up the hard close of the Websocket. Hope that makes sense!

[ellemouton]

makes sense! thanks for the explanation :)

[ViktorTigerstrom]

Just added some docs to the Disconnect to include info that the function expects that the onDisconnect is passed as an argument. I also reformatted the line breaks in the function.

[ellemouton]

tACK, LGTM!

Great investigation on this 👏 🏆