binance-exchange / node-binance-api

Node Binance API is an asynchronous node.js library for the Binance API designed to be easy to use.
MIT License
530 stars 144 forks source link

Websocket inconsistency #71

Closed michelesandroni closed 6 years ago

michelesandroni commented 6 years ago

Hello,

I've just noticed that, after subscribing to the candlestick websocket (for example) for symbol 'BTCUSDT' the library seems to subscribe to a websocket for each letter. In order words, when listing all websockets 60 seconds (with a timeout) after subscribing them, this is what is shown:

ws subscription: binance.websockets.candlesticks(['BTCUSDT'], '1m', (candlestickData) => { });

ws listing after 60 seconds: let endpoints = binance.websockets.subscriptions(); for ( let endpoint in endpoints ) { console.log('' + endpoint); }

console output: btcusdt@kline_1m b@kline_1m t@kline_1m c@kline_1m u@kline_1m s@kline_1m d@kline_1m

(tested on 0.4.8)

jaggedsoft commented 6 years ago

Thanks for your report! I can not reproduce this

binance.websockets.candlesticks(['BTCUSDT'], '1m', (candlestickData) => { });
setTimeout(()=>{
    let endpoints = binance.websockets.subscriptions();
    for ( let endpoint in endpoints ) {
        console.log(endpoint);
    }
}, 1000);

Result: image

jaggedsoft commented 6 years ago

you have inspired me to add a verbose option so we can troubleshoot events happening inside the library / add extra debug logging

jaggedsoft commented 6 years ago

I believe what you want is this:

    for ( let endpoint of endpoints ) {
        console.log(endpoint);
    }

edit: might not work

michelesandroni commented 6 years ago

strange, cause i just now used your repro steps as they are and this was the output in my system: btcusdt@kline_1m b@kline_1m t@kline_1m c@kline_1m u@kline_1m s@kline_1m d@kline_1m

jaggedsoft commented 6 years ago

Try console.log(endpoints); instead of the loop

michelesandroni commented 6 years ago

logging endpoints gives {b@kline_1m: WebSocket, t@kline_1m: WebSocket, c@kline_1m: WebSocket, u@kline_1m: WebSocket, s@kline_1m: WebSocket, …} b@kline_1m:WebSocket {domain: null, _events: {…}, _eventsCount: 5, _maxListeners: undefined, readyState: 1, …} btcusdt@kline_1m:WebSocket {domain: null, _events: {…}, _eventsCount: 5, _maxListeners: undefined, readyState: 1, …} c@kline_1m:WebSocket {domain: null, _events: {…}, _eventsCount: 5, _maxListeners: undefined, readyState: 1, …} d@kline_1m:WebSocket {domain: null, _events: {…}, _eventsCount: 5, _maxListeners: undefined, readyState: 1, …} s@kline_1m:WebSocket {domain: null, _events: {…}, _eventsCount: 5, _maxListeners: undefined, readyState: 1, …} t@kline_1m:WebSocket {domain: null, _events: {…}, _eventsCount: 5, _maxListeners: undefined, readyState: 1, …} u@kline_1m:WebSocket {domain: null, _events: {…}, _eventsCount: 5, _maxListeners: undefined, readyState: 1, …}

using 'of' instead of 'in' just throws an error

jaggedsoft commented 6 years ago

I'm sorry but I'm really at a loss here if we look at the candlesticks function:

candlesticks: function candlesticks(symbols, interval, callback) {
    for ( let symbol of symbols ) {
        let reconnect = function() {
            if ( options.reconnect ) candlesticks(symbol, interval, callback);
        };
        subscribe(symbol.toLowerCase()+'@kline_'+interval, callback, reconnect);
    }
},

Essentially all it's doing is this:

let symbols = ['BTCUSDT'];
for ( let symbol of symbols ) {
    console.log(symbol);
}
jaggedsoft commented 6 years ago

I'm adding verbose to the next release, shows when new subscriptions are made, etc You could try testing with reconnect: false in the options to eliminate if it's a reconnection issue

jaggedsoft commented 6 years ago

Maybe you can try the chart function? I prefer it over candlesticks

michelesandroni commented 6 years ago

chart shows the same behavior and also seem to use the same endpoint @kline_1m the reconnect flag doesnt seem to affect the behavior

michelesandroni commented 6 years ago

sorry, some more info about this: after closing and restarting nw.js, i cannot reproduce this one either. those zombie websockets seem to have been created at some point last night after the websocket was reconnected, but i can't tell exactly how or when. Let's keep this closed and I'll post more info once I have new details. Thanks!

michelesandroni commented 6 years ago

The verbose flag sounds like a really good idea!

jaggedsoft commented 6 years ago

Never heard of nw.js, it allows you to use this from the browser or what?

michelesandroni commented 6 years ago

It used to be called node-webkit, it's basically chromium + node and enables users to turn html5 into cross-platform executables among other things. Similar to electronjs I guess (I want to try that one soon). https://nwjs.io/

jaggedsoft commented 6 years ago

sweet thanks, keep me updated if you have any problems etc I'm probably gonna try electron myself, looks awesome!

michelesandroni commented 6 years ago

thank you!

michelesandroni commented 6 years ago

Now I can definitely reproduce the issue. The behavior is not always consistent (sometimes it generates the error immediately, sometimes not). / Output: index.html:105 Endpoint: btcusdt@kline_1m index.html:105 Endpoint: b@kline_1m index.html:105 Endpoint: t@kline_1m index.html:105 Endpoint: c@kline_1m index.html:105 Endpoint: u@kline_1m index.html:105 Endpoint: s@kline_1m index.html:105 Endpoint: d@kline_1m index.html:105 Endpoint: btcusdt@kline_1m index.html:105 Endpoint: b@kline_1m index.html:105 Endpoint: t@kline_1m index.html:105 Endpoint: c@kline_1m index.html:105 Endpoint: u@kline_1m index.html:105 Endpoint: s@kline_1m index.html:105 Endpoint: d@kline_1m index.html:105 Endpoint: btcusdt@kline_1m index.html:105 Endpoint: b@kline_1m index.html:102 Uncaught TypeError: Cannot read property 'code' of undefined at WebSocket. (C:\Test\node_modules\node-binance-api\node-binance-api.js:231:62) at WebSocket.emit (events.js:159:13) at WebSocket.finalize (C:\Test\node_modules\ws\lib\WebSocket.js:182:41) at WebSocket.terminate (C:\Test\node_modules\ws\lib\WebSocket.js:389:12) at Object.terminate (C:\Test\node_modules\node-binance-api\node-binance-api.js:770:8) /

class Tester { start() { this.binance = require('node-binance-api'); this.testWs(); } testWs() { var thiz = this; this.binance.candlesticks('BTCUSDT', '1m', (error, ticks, symbol) => { // ... thiz.binance.websockets.candlesticks(['BTCUSDT'], '1m', (candlestickData) => { // ... }); }, { limit: (267 + 14 + 1), endTime: Date.now() }); setTimeout(function() { let endpoints = thiz.binance.websockets.subscriptions(); for ( let endpoint in endpoints ) { console.log('Endpoint: ' + endpoint); thiz.binance.websockets.terminate(endpoint); } setTimeout(function() { thiz.testWs(); // test recursively ... }, 5000);
}, 5000); } }

let myTester = new Tester(); myTester.start();

jaggedsoft commented 6 years ago

This project is only designed for node js. I can not afford to research what I would need to do to make it work with nwjs or electron

michelesandroni commented 6 years ago

Hi Jaggedsoft,

please try saving the code from my previous comment as test.js, place it in some folder where you can install 'node-binance-api' via npm, then run "node test.js".

I just did it and i can repro the issue using node v8.9.4 (same results as nw.js).

jaggedsoft commented 6 years ago

I cannot reproduce this Possibly related to https://github.com/jaggedsoft/node-binance-api/issues/81