search

tarkah / tickrs

Realtime ticker data in your terminal 📈
MIT License
1.2k stars 59 forks source link

Crash with --time-frame 1D outside of trading hours #58

Closed wullewutz closed 3 years ago

wullewutz commented 3 years ago

tickrs hangs when time-frame is set to 1D when started before a certain time (maybe before trading start on NYSE?). To mitigate this, i can start with -t 1W, but as soon as i change to 1D inside the application, it also hangs.

Can reproduce this on Linux Arch with alacritty terminal as well as WSL2 (Ubuntu) on Windows Terminal. My timezone is GMT+1 (Berlin). At the time of filing this issue (8:20 GMT) the crash occurs. I observed the same yesterday morning. Later in the day, everything works as expected.

tarkah commented 3 years ago

Ahh yeah, this is probably happening sometime between midnight and 4am EST before the premarket opens. I'm sure the API switches over to the new day, but doesn't have data yet.

I don't think this used to happen before I added pre/post market data. It'd be helpful to know what the API request is returning so I can handle the error properly.

I'll post a few API links if you could test them for me at the same time tomorrow? I'll try to test tonight after 9pm PST to see if I can recreate.

Is the program crashing and if so, do you have the backtrace that gets printed?

wullewutz commented 3 years ago

No sorry, the program did not really crash but hangs and remains inresponsive. Have to kill it or better close the terminal since it messes up the cursor as well. I also noticed an increasing memory amount in the Windows task manager for the tickrs process. Could be a memory leak as well maybe?

I also cloned your repo run it with "cargo run" (Arch linux) to see if it behaves differently when build in debug mode, but it didn't change anything.

Feel free to post some API links. I'm glad to help and test tomorrow.

Awesome App by the way! I really like it so far.

tarkah commented 3 years ago

What version are you running? I released 0.10.0 yesterday with some major optimizations. You can get the latest version here: https://github.com/tarkah/tickrs/releases/tag/v0.10.1.

Assuming you're on >= 0.10.0 already, I'll need to figure out what's causing it to hang. What does your memory usage look like on startup vs when it halts? I'll try to throw it at Valgrind today to see if I can find any leaks. Though it's pretty difficult to have memory issues w/ Rust, it's not impossible. And I'm using some smart pointers in places that could in theory make it happen.

Note that memory usage should increase as you switch between time frames and as the day goes on. I cache the data from each time frame after its received for the first time so that switching back is more seamless. And obviously as the day goes on, more data is received from the API. But this should be a few MB at most I'd think.

With 4 tickers showing in summary mode, with all time frames cached, I'm at ~32MB. I'll try to keep this instance open all day and overnight to see if it balloons and becomes unresponsive.

Appreciate the issue so I can further improve this. Glad you are enjoying it!

tarkah commented 3 years ago

And for the links you can help me test during that timeslot...

Charting data

https://query1.finance.yahoo.com/v8/finance/chart/SPY?range=1d&interval=1m&includePrePost=true

Company info

https://query1.finance.yahoo.com/v10/finance/quoteSummary/SPY?modules=price,assetProfile

tarkah commented 3 years ago

Ahh, I see you're testing on Windows. Let me run a version there as well to see if there's any difference.

Edit: NVM, you're running under WSL2. I'm doing the same now for my test

tarkah commented 3 years ago

On windows / powershell, running w/ 4 stocks in summary mode, all time frames cached, I'm sitting at 7.4MB. Wow, memory usage is way lower on windows... interesting. Anyway, I'll let this one run all night as well and see if things change.

tarkah commented 3 years ago

Ok, I think I know what is deadlocking the program... I'll push out a PR w/ fix here shortly. It'd be great if you can test tonight on both versions to validate it fixes things.

tarkah commented 3 years ago

Ok, can you try running from source from https://github.com/tarkah/tickrs/pull/59 tomorrow? I'm 99.9% sure this was the problem as it appears I can create a "never ending" iterator when the API updates for the new trading day, which causes a deadlock since it's never ending. I've added some logic to prevent this from happening.

tarkah commented 3 years ago

And to follow up, there doesn't appear to be any memory leaks. I've been running for a few hours now and memory has only bumped up a single time by a few hundred KB, most likely due to a new Vector allocation to handle more pricing data.

wullewutz commented 3 years ago

Awesome! I'll pull tomorrow morning and report here.

tarkah commented 3 years ago

Great, thanks!

tarkah commented 3 years ago

I just tested old vs the PR and looks like it fixes it! Hopefully you see the same and I can get this merged tomorrow.

wullewutz commented 3 years ago

Here my testlog: Start time: 9:15 GMT Environment WSL2 (Ubuntu), rustc 1.49.0 (stable), Windows Terminal 1.5.10271.0

Pre-test with tickrs 0.10.0 (current master): open Terminal cargo run -- -s GME -t 1D (Observation: tickrs hangs, CPU > 13%, Memory usage rising fast to >100MB in <1min) close Terminal

Test with deadlock fix: open Terminal git checkout origin/fix/infinite-iterator-deadlock cargo clean cargo run -- -s GME -t 1D (Observation: tickrs starts and shows gui as expected. CPU@ <1%, Memory usage @12,5MB fix)

Conclusion: The issue seems to be fixed. Ready to merge. Well done!

tarkah commented 3 years ago

Awesome! Thanks for confirming

tarkah commented 3 years ago

@wullewutz New release is out w/ the fix:

https://github.com/tarkah/tickrs/releases/tag/v0.10.2