Trying to find cause of issue #2345 #2347
Conversation
|
Can I already verify this? I did this: As expected: traceback still there ... and traceback still there. Version seems correct: 3.8.0-develop [1c4a5da] Is this the correct method?
|
|
@sanderjo : That should be correct, could you try the updated version? |
sabnzbd/downloader.py
Outdated
| else: | ||
| # nothing returned, so there was a connection problem | ||
| logging.debug("%s: No successful IP connection was possible using happyeyeballs", self.host) | ||
| if not ip or cfg.load_balancing == 0 or len(self.info) == 1: |
There was a problem hiding this comment.
This is not correct. Missing ()
|
I tried, with a fresh clone from your repo. But SAB keeps hanging on www.google.nl. So far is has not fallen back / switched over to the next (working) newsserver. Version: 3.8.0-develop [28b18cb] |
|
Did you sett the Google server as optional? Otherwise it won't be disabled. Required: Will never give up trying to download an article unless it can connect to the server and it says it's missing. It should probably be changed in to a single drop down choice because selecting both makes no sense. |
Ah. Yes. Now I have. And it went OK. No more tracebacks. So people have to check Optional under Advanced? And the reason for that is to avoid falling back automaticall/default to blockservers?
|
|
I removed the Optional check, and no download (as expected), but ... nice Warnings. A pity those contains IP addresses (not the server name), but at least this should be an alert to a user (like me).
|
|
Good point about the IP address. It might be useful in some cases for debugging, but probably not for most users. I changed it to www.google.nl:8888 (142.250.74.3). Besides that it seems like this fixes at least two issues, the stack traces when using happy with this kind of non reachable server and making sure they aren't skipped if they are marked as required. I'm less sure about the implementation. Particularly the check for the warning message. Maybe it should be done elsewhere. My problem is that it's normally done in the section where NNTP status messages are checked and it never reaches that point. |
|
Much better! Do you understand why there are only two warnings?
|
|
It prints a warning if the error message has changed since last check and those strings contain different IPs. |
|
So in this case (unreachable server), there's no difference between Required and no special setting? |
There weren't, but there will be with this patch because it will no longer stop trying to fetch an article after 3 tries.
Yes. Maybe it would be better if it was consistent, but problems are a bit different. Some of the NNTP problems would cause SAB to hammer the server if we just ignored it and tried again when required is set. The unreachable server error has a fairly long time out and is done in separate threads so it doesn't cause the same problems if we don't give up. It also lets the user continue downloading if there are articles available for other servers. |
|
The Another difference is that |
|
Pff I am trying to understand the current error handling code and how we could improve it, but it's really some thick spaghetti. I feel there should be a way to consolidate this better and avoid having 3 spots where we handle errors. The difference between the |
This reverts commit ee2b2b2.
See #2345 for comments about the issue.