Wait for background currency fetch thread to finish before exiting #298
Conversation
When numbat is run with a quick-running --expression, it can finish evaluating the expression before the background currency fetch thread has finished. On Windows, this is problematic because the main thread does some post-main winsock shutdown tasks, which conflict with the background task that's still trying to fetch exchange rate info from the network. This commit keeps a reference to the currency fetching thread and joins on it before exiting.
|
This should resolve the sporadic CI test failures reported in #290 When reproducing the failures, I saw two types: the first is a crash from numbat.exe, and the second was a hung numbat.exe. In the case of the hung numbat, the main thread was blocked with a callstack that looks like this: Here we can see the main thread has exited, and now various exit handlers are running, including some windows socket shutdown functions. Meanwhile, the currency fetching thread is still running: |
|
Thank you very much for looking into this. That also explains why the test was flaky, because it probably depended on the timings of the HTTP requests. I do wonder, however, if this is the right fix. Imagine I have a slow internet connection (this is actually the case sometimes when I am behind a company proxy). Now my quick If that is the case, I'm not really sure how to proceed. We would probably have to send some kind of signal to the thread to close down immediately, without further blocking on the HTTP request. And that would probably involve a non-blocking solution + Maybe we should first find out if this is an actual, real-world problem :-) |
Yes, this is correct. In my tests, it takes about 500ms for the HTTP thread to finish. Obviously this could be much slower on a worse internet connection. Killing the background thread would be one option, but there isn't a simple way to do this. Sending a signal to the HTTP client to interrupt itself would be nice, but I don't know if attohttp supports this (I haven't looked). The most simple thing we could do is only spawn the background HTTP thread for interactive REPLs. If you use the |
Load on-demand for all other use cases.
Upon more reflection, this seems like the most sensible thing to do, so I just pushed a commit that implements this. PS, i love using hyperfine for this kind of stuff: |
I think this is a great solution. The only case where this would result in a performance regression would be if someone wrote a Numbat program (like in an actual file) that does some longer calculations without currencies and then suddenly requires the exchange rates. Previously, we would have loaded them in the background while the other calculation was running. But now we would need to fetch them on demand. But I think this is a far-fetched use case. But I definitely appreciate that we have a fast
Thank you for the feedback! Please notice that you are performing benchmarks with the debug build. The release build of numbat starts up much faster (~22 ms on my notebook. And much faster than that if you use |
|
I pushed a commit to re-enable the CI build for Window. This will now close #290, if I understand correctly. |
|
Agreed with your analysis above. In the future, we could implement a BTW, I think 32-bit windows CI builds were also impacted by this |
When numbat is run with a quick-running --expression, it can finish evaluating the expression before the background currency fetch thread has finished.
On Windows, this is problematic because the main thread does some post-main winsock shutdown tasks, which conflict with the background task that's still trying to fetch exchange rate info from the network.
This commit keeps a reference to the currency fetching thread and joins on it before exiting.