Closed dejan23 closed 1 year ago
That is quite an important find, thank you for raising this. This is definitely unexpected and seems to be an oversight on my part in how the user data stream teardown happens (also when the listenkey is for some reason invalid and a new one needs to be instantiated). I've scoped out an initial PR in #291 but do need to re-review it to ensure I haven't missed anything, including manually testing the workflow.
In case you'd like to help manually validate the fix is working as expected (via a local copy of the keepaliveteardown
branch), the timer is set to run every 50 minutes:
https://github.com/tiagosiebler/binance/blob/keepaliveteardown/src/websocket-client.ts#L880-L890
This is currently hardcoded but can easily be edited in a local copy of the branch.
Then it's just a matter of trying out the keep alive behaviour:
Otherwise I'll try to find time tomorrow to test out & validate these workflows. Thanks!
I think I found the issue. Maybe I did not provide all the relevant data.
I am doing this in Nestjs, node version v16
after I connect with a Testnet api key like this
new WebsocketClient({ api_key: apiKey, api_secret: apiSecret, beautify: true, });
and subscribe to
await wsClient.subscribeUsdFuturesUserDataStream(true);
after that when i want to disconnect i call .closeAll()
what i found out right now is when i disconnect i get this message
[
'Websocket connection closed',
{
category: 'binance-ws',
wsKey: 'usdmTestnet_userData_xzBGngxhyOz5BirjSj2Watu7x6TFzPHw3rtAzEn9JFqqG2RRxvt9YAujcsGlrTwW',
event: CloseEvent {
target: [WebSocket],
type: 'close',
wasClean: false,
reason: '',
code: 1006
},
wsConnectionState: 3
}
]
and then i see you are calling getContextFromWsKey(wsKey) function and in that function there is this part that is causing the issue
const [market, streamName, symbol, listenKey, ...otherParams] =
wsKey.split('_');
if you look at the wsKey from connection closed message and compare to wsKey.split('_') function you can see the problem. when i console.log({market, streamName, symbol, listenKey, ...otherParams}) i get
{
market: 'usdmTestnet',
streamName: 'userData',
symbol: 'xzBGngxhyOz5BirjSj2Watu7x6TFzPHw3rtAzEn9JFqqG2RRxvt9YAujcsGlrTwW',
listenKey: undefined
}
there is no symbol in wsKey so listen key gets assigned to symbol and in websocket-client.ts you are doing this
const { listenKey } = getContextFromWsKey(wsKey);
which is always undefined and it will never fall into this.teardownUserDataListenKey()
when i tried a quick fix like
const { symbol: listenKey } = getContextFromWsKey(wsKey);
everything was working fine, i did not get "Completed keep alive cycle for listenKey(...) in market(usdmTestnet)" message anymore
edit: Ive tested this on keepaliveteardown branch
This is interesting (and I do dislike my choice to embed information in the wskey), maybe a placeholder needs to be set when symbol exists in the key...even if it's something obvious & silly like noSymbol
as the "symbol" property. Thanks for the comments, will look to investigate more.
Regarding the userData conflict with this splitter:
const [market, streamName, symbol, listenKey, ...otherParams] = wsKey.split('_');
I noticed the futures user data subscribers are indeed the only ones that generate the wskey in a hardcoded way. For other methods I created a utility method that enforces more consistency:
const wsKey = getWsKeyWithContext(
market,
'userData',
undefined,
listenKey
);
Instead of what it was before, which (as you found) is missing a separator for the blank symbol:
const wsKey = [market, streamName, listenKey].join('_');
I'll include this fix in the branch, after thorough testing on other side effects this may cause. Just an update worth mentioning.
Hey @dejan23 - I've pushed more improvements around how the keep alive error handling works. Can you test again with the same branch with the latest changes in place? I found another missing cleanup for old ping/pong timers if the listenkey keep alive request fails repeatedly, but also cleaned up the workflow a fair bit. Would appreciate if you can also confirm the close and closeAll workflow is behaving as you expect. Thanks!
ive tested it. close and closeAll now work as expected.
ive changed keep alive timer from 50 minutes to 10 seconds just to check when i do close or closeAll and now it does not show "Completed keep alive cycle for listenKey(...) in market(usdmTestnet)" message after connection is closed anymore.
Good work! Thanks!
Thanks for the super quick confirmation! Glad to hear it. Will merge and release shortly, as v2.2.11.
hi there, ive noticed that after i connect to websocket with
new WebsocketClient({ api_key: apiKey, api_secret: apiSecret, beautify: true, });
after i close connection eather with .close(wsKey) or .closeAll() i keep getting "Completed keep alive cycle for listenKey(...) in market(usdmTestnet)" in terminal.
Is that expected?
if i have multiple users (with different api key) connect and later close the connection i keep getting multiple "Completed keep alive cycle" messeges for different listen keys