search

GetScatter / scatter-js

Importable JavaScript library that allows web applications to directly interface with Scatter Desktop, Classic and Mobile.
MIT License
262 stars 148 forks source link

Repeat websocket connections causes WalletAPI to utilize closed old websocket #156

Closed bradlhart closed 4 years ago

bradlhart commented 4 years ago

I added a uuid to each socket connection between scatterjs-core and Scatter Desktop to keep track of the connections and noticed that some WalletAPI functions are utilizing an old socket connection, thus causing "Websocket is already in CLOSING or CLOSED state." My testing was focused on WALLET_METHODS.getIdentity and I console logged the context and wallet.

context.socketService was using the newest websocket while wallet was using an old closed websocket. Adjusting getIdentity to use context.socketService instead of wallet got me further in the process without "Websocket is already in CLOSING or CLOSED state"

Related to the comments posted after this issue was closed: https://github.com/GetScatter/scatter-js/issues/69

Work in Progress: https://github.com/bradlhart/scatter-js/commit/9d5b9fcde232b8edcb0ea080e06cdd5b400397c5 https://github.com/bradlhart/ScatterDesktop/commit/41775b47bc495632bf9672c5781376294140480d

bradlhart commented 4 years ago

I'm having some luck by re-using the SocketService rather than closing it before the next wallet.connect(). Although, I think there is a lot more work needed to ensure the SocketService.socket connection is still valid. Additionally, if the connection wasn't valid and a new socketService is needed, this wouldn't solve the root cause issue.

Not opening a PR yet as I don't believe this is a full implementation, but here's the most recent progress: https://github.com/GetScatter/scatter-js/commit/15c822de324802dcb40a077271aca5a6ceaf66e6

nsjames commented 4 years ago

Are you perhaps calling .connect() multiple times?

nsjames commented 4 years ago

Here, this commit should fix this issue: https://github.com/GetScatter/scatter-js/commit/6f6204d75554d75743876ce983367658a1c37546

It looks like the WalletAPI was holding a reference to the first set socket service instance instead of pulling the current one.

bradlhart commented 4 years ago

Works great! Thanks!

nsjames commented 4 years ago

Funny, i've been searching for this issue for a while and hoped someone else would hop on it. Thanks for the direction!