GNOME Bugzilla – Bug 725386
Driver downloads interferes with wizard navigation
Last modified: 2016-09-20 08:15:55 UTC
0) Get rid of ~/.cache/gnome-boxes/drivers 1) Click on "New" button in main interface 2) Pick a windows image (win7 in my tests), -> Boxes shows a screen where it downloads drivers 3) Go back while the download is in progress -> I've hit various weirdness after that, for example: 4) Pick a live image (f19 64bit in my testing) 5) Click on the 'customize' button 6) Wait a bit until the drivers download finishes 7) Click on 'back' button -> we end up on the "configuration" screen rather than on "summary"
Hmm.. I can reproduce some other weirdness here too. Seems we are not cancelling the download on 'back' button. Even worse, the same happens on Cancel button. Easier way to reproduce is to do the following instead after step 3 above: 4) wait while download finishes (doesn't take very long) 5) you'll be automatically taken to 'setup' page for win7.
We have a prepare_cancellable that is used for downloading media. It is already called when the back/cancel buttons are clicked so downloads are aborted. So it shouldnt be difficult to get the downloads cancelled - I'll do this.
Created attachment 288838 [details] [review] wizard: Instantiate prepare_cancellable only once It is useless to reinstantiate it several times. This also lowers code complexity.
Created attachment 288839 [details] [review] downloader: Use cancellable for fetch_os_logo
Created attachment 288840 [details] [review] unattended-installer: Use cancellable for download The prepare_cancellable from the Wizard is passed through to the setup_driver function. If we use it for driver downloading the downloads will get cancelled correctly when pressing the back/cancel button in the wizard.
Please note that I wasn't yet able to test if these patches fix this bug - but they are needed anyway so I did upload them.
Ok, I don't have a windows image at hand - Zeeshan, could you test if these patches fix this?
(In reply to comment #7) > Ok, I don't have a windows image at hand - Zeeshan, could you test if these > patches fix this? Nope, they don't fix the bug at least when you hit 'back' button in preparation.
Review of attachment 288838 [details] [review]: ::: src/wizard.vala @@ -590,3 @@ page = WizardPage.SOURCE; - } finally { - prepare_cancellable = null; You can't just re-use cancellable after its cancelled. You gotta ensure its reset before next use.
Review of attachment 288839 [details] [review]: Better log: downloader: Make fetch_os_logo() cancellable. Patch looks good.
Review of attachment 288840 [details] [review]: * same nit about shortlog. How about: unattended-installer: Cancellable driver download * Patch is good otherwise but as I mentioned it doesn't work for some reason.
Review of attachment 288838 [details] [review]: ::: src/wizard.vala @@ -590,3 @@ page = WizardPage.SOURCE; - } finally { - prepare_cancellable = null; Ok, since we don't use is_cancelled for the prepare_cancellable I'll reset it right after use if thats ok for you.
Review of attachment 288838 [details] [review]: ::: src/wizard.vala @@ -590,3 @@ page = WizardPage.SOURCE; - } finally { - prepare_cancellable = null; Sure.
Created attachment 289043 [details] [review] wizard: Instantiate prepare_cancellable only once It is useless to reinstantiate it several times. This also lowers code complexity.
Created attachment 289044 [details] [review] downloader: Make fetch_os_logo() cancellable
Created attachment 289045 [details] [review] unattended-installer: Cancellable driver download The prepare_cancellable from the Wizard is passed through to the setup_driver function. If we use it for driver downloading the downloads will get cancelled correctly when pressing the back/cancel button in the wizard.
Review of attachment 289044 [details] [review]: I need to catch the CANCELLED exception somewhere.
Created attachment 289052 [details] [review] downloader: Make fetch_os_logo() cancellable
Created attachment 289053 [details] [review] unattended-installer: Cancellable driver download The prepare_cancellable from the Wizard is passed through to the setup_driver function. If we use it for driver downloading the downloads will get cancelled correctly when pressing the back/cancel button in the wizard.
these patches actually work this time by the way.
Review of attachment 289043 [details] [review]: ack
Review of attachment 289052 [details] [review]: good otherwise. ::: src/wizard.vala @@ +337,3 @@ prep_media_label.label = install_media.os.name; + try { + Downloader.fetch_os_logo.begin (installer_image, install_media.os, 128, prepare_cancellable); You get a compiler warning for this. :) You can't spawn off an async call with begin and still be able to catch errors from it like this. You either need to yield to it or pass a callback and .end the async call from the callback to be able to catch the error.
Review of attachment 289053 [details] [review]: ::: src/unattended-installer.vala @@ +495,3 @@ additional_devices.add_all (driver.get_devices ()); + } catch (IOError.CANCELLED cancel_error) { + throw cancel_error; you need to declared the function that i can throw errors first. For this you get a compiler warning and a runtime warning when you cancel. :) ::: src/wizard.vala @@ +356,3 @@ + yield install_media.prepare (prepare_media_progress, prepare_cancellable); + } catch (IOError.CANCELLED cancel_error) { + debug ("Cancelled driver downloading for %s!", install_media.os.name); 1. no need for "!". :) 2. this code doesn't know anything about driver downloading. I'd just ditch this debug. Feel free to add it to the part that does the driver downloading in the chain..
Review of attachment 289052 [details] [review]: ::: src/wizard.vala @@ +337,3 @@ prep_media_label.label = install_media.os.name; + try { + Downloader.fetch_os_logo.begin (installer_image, install_media.os, 128, prepare_cancellable); Then I'd drop all changes from this file. Since we just invoke fetch_os_logo and then don't care whether it succeeds or not I think this invocation doesnt need the cancellable. Let it finish its small download rather than having two asynchronous things adjusting the wizard page - wich would lead to a similar bug to the one I'm trying to fix here. The question is if you want this patch anyway so the function is _able_ to handle the cancellable which makes the whole API of the downloader cancellable.
Created attachment 289117 [details] [review] downloader: Make fetch_os_logo() cancellable
Created attachment 289118 [details] [review] unattended-installer: Cancellable driver download The prepare_cancellable from the Wizard is passed through to the setup_driver function. If we use it for driver downloading the downloads will get cancelled correctly when pressing the back/cancel button in the wizard.
Review of attachment 289052 [details] [review]: ::: src/wizard.vala @@ +337,3 @@ prep_media_label.label = install_media.os.name; + try { + Downloader.fetch_os_logo.begin (installer_image, install_media.os, 128, prepare_cancellable); yeah, its fine if this is not cancellable (at least for now) but we should ensure that logo on preparation is no longer populated. Imagine this scenerio: 1. You selected a Fedora ISO. 2. In preparation, fedora logo starts to be downloaded. Lets assume it takes too long for some server issues. 3. you go back and choose an Ubuntu ISO. 4. In preparation, the pending fedora logo is downloaded before the new ubuntu logo download can finish. You'll see fedora logo in preparation and that wouldn't just be wrong but it might get us into legal problems. :)
Review of attachment 289117 [details] [review]: NACK, no need to make it cancellable if we are not going to make use of this feature.
Review of attachment 289117 [details] [review]: We dont need that patch at all then. We invoke fetch_os_logo, prepare the rest. If the user cancels and goes to another system I don't see how he'll get the wrong logo since fetch_os_logo doesnt put the logo there. So I'd pledge for just reject that patch without replacement.
Review of attachment 289118 [details] [review]: You are making the whole prepare_media chain cancellable not just driver download. I'll recommend updating the shortlog to reflect that. I don't think talking about internal variables/fields (prepare_cancellable here) w/o indication of what they are, make a very readable log. I'd just write: "Ensure InstallerMedia.prepare is cancellable so Wizard can cancel it on its back/cancell buttons being clicked. This fixes the issue of Wizard jumping to setup page after driver download is finished even if user has cancelled their request to create the corresponding VM." ::: src/installer-media.vala @@ +84,3 @@ public virtual void set_direct_boot_params (DomainOs os) {} public virtual async void prepare (ActivityProgress progress = new ActivityProgress (), + Cancellable? cancellable = null) throws IOError.CANCELLED {} can we take this opportunity to align the parameter names, if they weren't already? ::: src/wizard.vala @@ +348,3 @@ + try { + yield install_media.prepare (prepare_media_progress, prepare_cancellable); + } catch (IOError.CANCELLED cancel_error) { Don't think there is much point in communicating the error back to caller here if the caller (Wizard) is the one that cancelled. Why not just do everything you do in the catch block where prepare_cancellable in cancelled? Not communicating the cancelled error all the way back would also make this patch slimmer and more readable. :) @@ +350,3 @@ + } catch (IOError.CANCELLED cancel_error) { + debug ("Cancelled media preparation for %s.", install_media.os.name); + page = WizardPage.SOURCE; didn't notice this part before. Why do we need this line if we didn't before?
Review of attachment 289117 [details] [review]: > since fetch_os_logo doesnt put the logo there. It does. The paramater to this method is the GtkImage we use to show logo on. I think we need this.
Review of attachment 289118 [details] [review]: ::: src/wizard.vala @@ +348,3 @@ + try { + yield install_media.prepare (prepare_media_progress, prepare_cancellable); + } catch (IOError.CANCELLED cancel_error) { I am not sure if I'm understanding you right. install_media.prepare () takes a cancellable, thus is expected to throw an IOError.CANCELLED exception if it gets cancelled, we agree on that? The whole taking a cancellable without throwing the CANCELLED exception thing was very weird to me from the beginning. Besides I can't return this method in the catch block where the prepare_cancellable is cancelled.
Review of attachment 289118 [details] [review]: ::: src/wizard.vala @@ +348,3 @@ + try { + yield install_media.prepare (prepare_media_progress, prepare_cancellable); + } catch (IOError.CANCELLED cancel_error) { > install_media.prepare () takes a cancellable, thus is expected to throw an > IOError.CANCELLED exception if it gets cancelled, we agree on that? No, we don't. There is no rule or convention about that except that maybe keeping it similar to gio API but i don't see any reason to since we don't throw any other error anyway. > Besides I can't return this method in the catch block where the prepare_cancellable is cancelled. Ah ok, didn't think of that. Alternative would be that InstallerMedia.prepare can return boolean. then this code will simply become: if (! yield install_media.prepare..) return; I know this will also require function signatures but I think it would be cleaner solution in the sense that caller doesn't need to know why prepare() didn't successfuly complete its work.
Created attachment 289290 [details] [review] wizard: Instantiate prepare_cancellable only once It is useless to reinstantiate it several times. This also lowers code complexity.
Created attachment 289291 [details] [review] nstaller-media: prepare() return true on success This way InstallerMedia.prepare can be cancelled, return false when cancelled and the wizard may stop continuing with the usual workflow. This fixes the issue of Wizard jumping to setup page after driver download is finished even if user has cancelled their request to create the corresponding VM.
Review of attachment 289291 [details] [review]: ::: src/unattended-installer.vala @@ +502,2 @@ } catch (GLib.Error e) { debug ("Failed to make use of drivers at '%s': %s", driver.get_location (), e.message); maybe it makes sense to return false here too? If something wents wrong with one driver, is this enought for the installation to fail?
Review of attachment 289291 [details] [review]: typo: "nstaller-media" in shortlog. Don't think its correct English to say "This way InstallerMedia.prepare can be cancelled, return false..". "This way if InstallerMedia.prepare is cancelled, it returns false and Wizard stops continuing with rest of preparation". ::: src/unattended-installer.vala @@ +502,2 @@ } catch (GLib.Error e) { debug ("Failed to make use of drivers at '%s': %s", driver.get_location (), e.message); don't think so.As you can see above, we don't add the driver to be used if there is any error. ::: src/wizard.vala @@ +346,3 @@ prepare_media_progress.bind_property ("info", prep_status_label, "label"); + if (!yield install_media.prepare (prepare_media_progress, prepare_cancellable)) { + debug ("Failed media preparation for %s.", install_media.os.name); This will be a wrong message for the case of cancellation. Just put a debug at the point of failure.
Review of attachment 289290 [details] [review]: ::: src/wizard.vala @@ +211,3 @@ public void cleanup () { + prepare_cancellable.cancel (); + prepare_cancellable.reset (); This is wrong! cancellation unfortunately is not sync so you can end up reseting the cancellable before its async users have had a chance to deal with cancellation. @@ +362,2 @@ } I'd reset the cancellable in here. @@ +611,3 @@ back_button.clicked.connect (() => { + prepare_cancellable.cancel (); + prepare_cancellable.reset (); same comment about resetting immediately after cancellation.
Created attachment 289398 [details] [review] wizard: Instantiate prepare_cancellable only once It is useless to reinstantiate it several times. This also lowers code complexity.
Created attachment 289399 [details] [review] installer-media: prepare() return true on success This way if InstallerMedia.prepare is cancelled, it returns false and Wizard stops continuing with rest of preparation. This fixes the issue of Wizard jumping to setup page after driver download is finished even if user has cancelled their request to create the corresponding VM.
Review of attachment 289398 [details] [review]: ACK though I'll feel better with " a bit" appended to "code complexity". :)
Review of attachment 289399 [details] [review]: IMO this patch should be divided a bit. Each patch for each function starting to return bool and last patch that acts on prepare return false.
Created attachment 289463 [details] [review] wizard: Instantiate prepare_cancellable only once It is useless to reinstantiate it several times. This also lowers code complexity a bit.
Created attachment 289464 [details] [review] installer-media: prepare() return true on success This allows callers to handle cancellation/failure.
Created attachment 289465 [details] [review] wizard: Handle return value of prepare() This way if InstallerMedia.prepare is cancelled, it returns false and Wizard stops continuing with rest of preparation. This fixes the issue of Wizard jumping to setup page after driver download is finished even if user has cancelled their request to create the corresponding VM.
Created attachment 289467 [details] [review] installer-media: prepare() return true on success This allows callers to handle cancellation/failure.
Created attachment 289468 [details] [review] wizard: Handle return value of prepare() This way if InstallerMedia.prepare is cancelled, it returns false and Wizard stops continuing with rest of preparation. This fixes the issue of Wizard jumping to setup page after driver download is finished even if user has cancelled their request to create the corresponding VM.
Review of attachment 289463 [details] [review]: ack if you have tested it.
Review of attachment 289467 [details] [review]: ::: src/installer-media.vala @@ +84,2 @@ public virtual void set_direct_boot_params (DomainOs os) {} + // Returns true on success, false otherwise Redundant comment. Its a usual convention for a function returning a bool to indicate success on returning true. If return value indicated something els, then it wouldn't be obvious and would be good to have such a comment. ::: src/unattended-installer.vala @@ +462,3 @@ private delegate void AddUnattendedFileFunc (UnattendedFile file); + private async bool setup_drivers (ActivityProgress progress, Cancellable? cancellable = null) { You misunderstood me. I meant even change of signature of each function going into separate patch but i guess this is fine too since other functions are private any way and can have any signature..
Review of attachment 289468 [details] [review]: Patch itself is good but I think you should specify 'InstallerMedia.prepare' in shortlog. Now to keep it short, the '()' are not as important (props/fields don't have return values so its very much obvious from context its a function/method) and you can shorten 'return value' to 'ret val' as its going to be pretty obvious to programmers what that means.
Pushed these with minor changes I pointed out in the last review. Attachment 289463 [details] pushed as 0862e67 - wizard: Instantiate prepare_cancellable only once Attachment 289467 [details] pushed as ea755cb - installer-media: prepare() return true on success Attachment 289468 [details] pushed as a72de39 - wizard: Handle return value of prepare()
(In reply to comment #51) > Pushed these with minor changes I pointed out in the last review. Actually, it turns out that I pushed these patches unmodified, just cause I forgot to do a `git rebase --continue` after modifying last commit. :( I'll rebase my unpushed changes and push them..