Page MenuHomeLubuntu

Updated automirror for urllib.request

Authored by SBanya on Oct 2 2018, 12:34 AM.



Updated automirror for urllib.request

Test Plan

Verify basic function in US, but then also have non-US users test for functionality, especially paying attention to time.

More specifically, this patch should be able to be applied directly to /usr/lib/(i138-linux-gnu|x86_64-linux-gnu)/calamares/modules/automirror/ in a live system, then run the installer and see what happens.

Diff Detail

rCALASETTINGS Ubuntu Calamares Settings
Automatic diff as part of commit; lint not applicable.
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

SBanya requested review of this revision.Oct 2 2018, 12:34 AM
SBanya created this revision.
wxl retitled this revision from # Enter a commit message. # # Changes: # # common/modules/automirror/ # debian/changelog Updated automirror for urllib.request to Updated automirror for urllib.request.Oct 2 2018, 12:37 AM
wxl added a comment.Oct 2 2018, 12:46 AM

I'm not sure this is sufficient to fix the problem, but some testing will help shed some light on it. Since it sounds like this is a common gotcha with urllib, this may very well be the issue.

My gut tells me timeouts, though, given the fact that it works sometimes and not others. So I encourage you in a further commit to add an arbitrary timeout and a simple enough error handling to perhaps eschew the need for a particular mirror and instead use some default. @tsimonq2 what would be a good default?

Then as bonus points, add error handling to handle other errors.

Good work and congrats on your first commit! 🌟

wxl edited the test plan for this revision. (Show Details)Oct 2 2018, 1:07 AM
tsimonq2 changed the visibility from "All Users" to "Public (No Login Required)".Oct 2 2018, 10:05 AM

The problem here isn't that the urllib call is wrong (although that could certainly be possible) but the problem is that there's a race condition that needs to be solved.

Take a look:

if libcalamares.globalstorage.value("hasInternet"):
    country = getcountry()
    prefix = getmirror(country)
    prefix = ""

This function is only called if Calamares thinks there's internet. That works in most cases, but not when the internet is disconnected in between Calamares starting and getting to automirror.

You need to catch the urllib exception, and default to setting prefix to an empty string if there are problems. This DTRT everywhere, and isn't much of a hack.

Oh, for what it's worth, the diff is fine, but I would expect more here to actually fix the problem.

wxl added a comment.Oct 3 2018, 4:34 PM

@tsimonq2 the folks in #python made this suggestion as one of the possible issues.

On the other hand, perhaps you are right as the crash ends with a URLError:

File "/usr/lib/python3.6/urllib/", line 1320, in do_open
  raise URLError(err)

This is distinct from HTTPErrors and is a subclass of OSError so I would take it to mean a protocol issue of some kind, of which lack of networking certainly could be.

On the other hand, I just ran it in a VM with no networking— even after selecting an overseas location in hopes it would trigger some weird behavior from the mirror— and everything worked fine.

Thus, I don't think it's a no-networking issue anymore than I think it's a timeout issue.

Still, we could use some additional error handling to remove doubts.

tsimonq2 requested changes to this revision.Oct 4 2018, 7:25 AM

Right, we do need additional error handling here before I'm comfortable landing this.

Thanks again for your work @SBanya!

This revision now requires changes to proceed.Oct 4 2018, 7:25 AM
wxl updated this revision to Diff 101.EditedOct 10 2018, 10:42 AM

I took @SBanya's changes and:

  • applied them to both getcountry() and getmirror()
  • made PEP8 happy with them

Previous to that, he did test the basic function of these, i.e. that they didn't break anything.

tsimonq2 accepted this revision.Oct 10 2018, 8:55 PM
This revision is now accepted and ready to land.Oct 10 2018, 8:55 PM
This revision was automatically updated to reflect the committed changes.