ctubio / tribeca

Self-hosted crypto trading bot (automated high frequency market making) in node.js, angular, typescript and c++
https://127.0.0.1:3000
Other
95 stars 26 forks source link

fixing ok coin #41

Closed Camille92 closed 7 years ago

Camille92 commented 7 years ago

Same issue and same solution than for -> https://github.com/ctubio/tribeca/issues/21

coveralls commented 7 years ago

Coverage Status

Coverage remained the same at 55.255% when pulling eef3a6786f5929b7ac9ae341de385eb8fcbf5214 on Camille92:patch-14 into ccd419ba72f0c239697f01cc714569a816501acd on ctubio:master.

ctubio commented 7 years ago

Im sorry i think is a bad idea to remove this exception, because thanks to this exception we know that the application is not understading a message from okcoin api, that is, our communication is not perfect and should be fixed.

If we remove this exception, we will lost sight of faulty messages, and we will ignore them instead of notice them and fix them.

A fautly communication i believe is a good reason to stop the application, because any consequences could arraise if we dont communicate 100% right. To make this situations noticeable and as harmless as possible, the applicatoin throws a exception and stops all actions (but foreverresumes the party saddly in this case).

Anyway, we should fix the wrong communication message instead of ignore it, If you agreee can you please paste you log lines as new issues everywhere it says: Error parsing msg removing sensitive data? Will try to fix them all'¡ (btw look for it in my logs and i didnt found any, so i didnt reproduce this situation yet i think)

Camille92 commented 7 years ago

I agree with your description,

But if I understand well, we have an issue here as we call the throw inside its own catch so it's not solving the issue. Is something like that better ?

    var e = new Error('example');
    throw e;
    }
    catch (e) {
        this._log.error(e, "Error parsing msg", raw);

And by getting rid of the throw we still have the logs working well !

ctubio commented 7 years ago

in this case of faultry communication the throw should be inside the catch, so to crash the application and stop all actions.

usually in all the app we throw inside the try as in your code, so the code tries to run and if a exception is thrown it will be catched in the ´catch´ block and do something reasonable to avoid crash the application and continue normal execution - this is what we do 99% of thew times.

but this is not the case here, here we need to stop the application cos the communication with the api was faulty, and this doesnt have any reasonable hotfix so we don't wanna catch this exception thrown, we just want to give up, that is, do not put throw inside a try cos there is nothing we can try.

let me know if you require more info'¡

Camille92 commented 7 years ago

Oh ok it's very more clear.

But I don't understand why is there not anything to try before crashing like: trying to reconnect or waiting 5 or 10 segs!

ctubio commented 7 years ago

well, thats what is currently doing: throwing an uncatched exception does the application to exit, but then forever notices it and restarts the application, so effectively it should reconnect after a few seconds once this code path is hit, leaving in the logs some Error parsing msg *whatever* lines.

Camille92 commented 7 years ago

Ok you convinced me! I send you logs when I get some. :)

But the problems are the same as before: Some crashes from time to time and ghost orders!

ctubio commented 7 years ago

i found ghost orders to be the consequence of a exit/reconnect, i will try to monitor the logs but if you found any also feel free to paste your log line too'¡

many thanks for your time''