lightninglabs / lightning-node-connect

MIT License
78 stars 22 forks source link

Fix WASM-client disconnection error #103

Closed ViktorTigerstrom closed 7 months ago

ViktorTigerstrom commented 8 months ago

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 commented 7 months ago

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 :)!

ViktorTigerstrom commented 7 months ago

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 commented 7 months ago

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 commented 7 months ago

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: https://github.com/lightninglabs/lightning-node-connect/blob/d8c9f92056e2b5a0c908514b2159d01738f7afb7/example/index.html#L56-L58 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.

ViktorTigerstrom commented 7 months ago

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.