search

FAForever / client

FAF Python Client
GNU General Public License v3.0
75 stars 88 forks source link

Stalled download can cause client to become unresponsive #853

Open thetrav opened 7 years ago

thetrav commented 7 years ago

I experienced... write here what happened to you or what the current behaviour is. My network is pretty crappy.
Some downloads stop receiving packets without the tcp connection being severed

In some situations this causes the FAF lobby to become un-responsive during mod updates, and prevents me from playing :(

how to reproduce if explanation is needed to reproduce the problem, add it here It's super hard to reproduce. You basically need a way to stop packets from returning from the server part way through a download.

I expected... write here what you think the best behaviour should be Ideally the socket timeout should match the urllib timeout that you've set (20 seconds) then I'd get a normal "download failed" message, and I could retry.

Where the bug is / What should be done if you have a suggestion for fixing the issue, add it here. I'm reasonably certain the problem lies in the block: https://github.com/FAForever/client/blob/develop/src/fa/updater.py#L344-L353 specifically the read method call on 348 won't respect the timout set by UrlLib, given the read call blocks indefinitely, the process events method in QT is never called, so the UI becomes unresponsive. It's discussed in this stack overflow question, and the accepted response provides a mechanism for setting that timeout https://stackoverflow.com/questions/811446/reading-a-stream-made-by-urllib2-never-recovers-when-connection-got-interrupted

duk3luk3 commented 7 years ago

Thank you for the report. You're totally correct that that is a bad implementation.

What we should do here is remove urllib and use the DownloadManager that's based on QNetworkAccessManager after overhauling that a little bit (which I will do for #852 ).

Alternatively using the NetworkAccessManager directly as done for the client updater would be fine too.

If you're interested, feel free to fork the code and take a stab yourself!

Wesmania commented 7 years ago

Rather than using a timeout, we should just read data asynchronously. The entire updater is synchronous legacy code and we're planning to replace it. duk3luk3, maybe we can use (refactored) FileDownloader code to fix this up for now?