Closed gustafg closed 6 years ago
Nice catch! It shouldn't be too much of a hassle to fix it, but of course it's always better to make sure that the solution is accurate and thorough.
Unless you'd like to provide a solution yourself, I can look into it in a day or two.
I've tested { "maxInactiveMinutes": 1, ...}
this morning (and confirmed the unit is minutes). Just didn't have time to comment before now. I will attempt to fix it today. If I manage I will send a pull request.
I've made a change here now, found an additional issue (clearInterval() vs clearTimeout()): https://github.com/gustafg/avanza/commit/f85852613bbbeb8733c0eadeef378adcb499ad34
I didn't create a new pull request now because I want to think/test some more, specifically I wonder about the following things:
Any comment on your opinion on these things would be helpful, I have a slight tendency getting stuck in details sometimes. :)
Thanks for making this thing btw! Forgot to say that initially.
Awesome, thanks! Since you did not create a pull request I decided to just push up the changes myself.
Some thoughts regarding your questions:
this.call
as well. Though I'd like to look into 2 first, so to see if there's a need to rethink the re-authentication in its entirety. Yes, sorry, I didn't want to create a pull request until I had time to investigate the side effects and I became busy at work so had no energy left for this the past week.
What I meant with point 1 was like this: Before this commit the session timeout was (potentially) 60 days, assuming Avanza do not silently set it to some max value like 24 hours. This means that even though the API created a new session every 26 seconds the quote feed session cookie or what to call it should be valid for 60 days and hence you would not see any errors on the quote feed unless it ran for more than 60 days (which is probably unlikely). After this commit, hypothetically I suppose the quote feed could die or not be able to update itself after 24 hours.
On a side note, it seems you can set a lower timeout than 30 minutes, it's just not presented by the web ui and possibly the Iphone/Android clients, that's why I selected 30 minutes as the min value. Note though that if you set it to lower than 2 you will get a reauthentication scheduling loop. That's the primary reason why I inserted that MIN value (that you did not commit -- I believe) and aligned it with what Avanza proposes to use.
I've done some testing now, it works as follows:
Test 1 (session timeout = 2min, automatic re-authentication disabled)
[{"channel":"/meta/disconnect","successful":true}]
[{"advice":{"reconnect":"none"},"channel":"/meta/connect","id":"8","successful":true}]
[{"advice":{"interval":0,"reconnect":"handshake"},"channel":"/meta/connect","id":"9","error":"402::Unknown client", "successful":false}]
Test 2 (session timeout = 2min, automatic re-authentication disabled)
Test 3 (session timeout = 2min, automatic re-authentication enabled)
Personally I want test 3 to succeed regardless if it could hypothetically be more convenient to ping the same session over and over so I intend to develop something to support that unless someone else does it first.
Actually, when I say quote feed above I mean the web socket meta channels, I haven't really been observing any quotes since market is closed. It's even possible that the quote feed setup will be continue to be served. I will have to investigate further tomorrow.
I've tested more now that the market is open. Result is same as last night. The web socket dies after 24 hours (default) and cannot recover.
I've noticed another issue when I've tried to reconnect the web socket. It seems that once you do a new /meta/handshake on the websocket you get response amplification, meaning if you have done 3 /meta/handshake it will repeat each incoming message from Avanza 3 times (or Avanza sends each message 3 times). This seems to happen regardless if I invoke a new TCP connection towards Avanza or attempt to re-use the existing one. I'm not proficient in websockets though and my debugging statements may not be perfect. I'm sure something weird is going on though.
Disregard this, it was me.
I'm closing this issue as the problem described by the issue description actually is solved.
Re-opening because I noticed the fix you made was only applied to your dev branch.
The re-authentication is happening more often than intended (every 26 seconds). This is because
MAX_INACTIVE_MINUTES = 60*60*24
(which is number of seconds per day, not minutes). Which is later used indirectly for scheduling the re-authentication timeout:Additionally API send
{ "maxInactiveMinutes": 86400, "password": ..., "username": ... }
to Avanza, which hypothetically I suppose could be valid, but it looks very odd.It is my intention to debug this myself and send a pull request eventually but I thought maybe you already know exactly how this works so then I don't really need to do any investigation concerning this.