OldSquirrelForWindows icon indicating copy to clipboard operation
OldSquirrelForWindows copied to clipboard

Access violation when checking for updates

Open peters opened this issue 11 years ago • 20 comments

@shiftkey

Version: 0.7.2

Please note that the update directory is empty, and i just wanted to test what Shimmer does in that case. Appearently it results in a epic crash.

Also access violation is also observed in these scenarios:

  1. You specify an invalid url (borked URI)
  2. The folder containing updates does not exist

So.... This is not quite ready for prime time i guess :fire_engine:

using (var updateManager = new UpdateManager(OdinKapitalSettings.UpdateUrl, "odinkapital", FrameworkVersion.Net45))
            {
                btnSettingsCheckForUpdates.Enabled = false;
                var updateInfo = await updateManager.CheckForUpdate();
                if (updateInfo == null || !updateInfo.ReleasesToApply.Any())
                {
                    MessageBox.Show(this, "No updates available.");
                    btnSettingsCheckForUpdates.Enabled = true;
                }
                else
                {
                    if (DialogResult.OK != 
                        MessageBox.Show(this, "A new version is available, do you want to continue?", "New update available", MessageBoxButtons.OKCancel))
                    {
                        return;
                    }
                    var releases = updateInfo.ReleasesToApply;
                    await updateManager.DownloadReleases(releases);
                    await updateManager.ApplyReleases(updateInfo);
                    Application.Restart();
                }
            }

Log

[WARN][2013-10-13T12:21:37] UpdateManager: Failed to load local release list: System.IO.FileNotFoundException: Could not find file 'C:\Users\peters\AppData\Local\odinkapital\packages\RELEASES'.
File name: 'C:\Users\peters\AppData\Local\odinkapital\packages\RELEASES'
   at System.IO.__Error.WinIOError(Int32 errorCode, String maybeFullPath)
   at System.IO.FileStream.Init(String path, FileMode mode, FileAccess access, Int32 rights, Boolean useRights, FileShare share, Int32 bufferSize, FileOptions options, SECURITY_ATTRIBUTES secAttrs, String msgPath, Boolean bFromProxy, Boolean useLongPath, Boolean checkHost)
   at System.IO.FileStream..ctor(String path, FileMode mode, FileAccess access, FileShare share, Int32 bufferSize, FileOptions options, String msgPath, Boolean bFromProxy)
   at System.IO.FileStream..ctor(String path, FileMode mode, FileAccess access, FileShare share, Int32 bufferSize, Boolean useAsync)
   at System.IO.FileInfo.OpenRead()
   at System.IO.Abstractions.FileInfoWrapper.OpenRead()
   at Shimmer.Client.UpdateManager.checkForUpdate(Boolean ignoreDeltaUpdates, IObserver`1 progress)
[INFO][2013-10-13T12:21:37] UpdateManager: Downloading RELEASES file from http://www.odinkapital.no/shimmer/updates

Partial stack trace

A first chance exception of type 'System.IO.FileNotFoundException' occurred in mscorlib.dll
A first chance exception of type 'System.Exception' occurred in Shimmer.Core.dll
A first chance exception of type 'System.Exception' occurred in mscorlib.dll
The thread 'Event Loop 1' (0xb94) has exited with code 0 (0x0).
A first chance exception of type 'System.Exception' occurred in mscorlib.dll
A first chance exception of type 'System.Reflection.TargetInvocationException' occurred in mscorlib.dll
A first chance exception of type 'System.IO.FileNotFoundException' occurred in mscorlib.dll
A first chance exception of type 'System.IO.FileNotFoundException' occurred in mscorlib.dll
'odinkapital.vshost.exe' (Managed (v4.0.30319)): Loaded 'Microsoft.GeneratedCode'
The program '[7008] odinkapital.vshost.exe: Program Trace' has exited with code 0 (0x0).
The program '[7008] odinkapital.vshost.exe: Managed (v4.0.30319)' has exited with code -1073741819 (0xc0000005) 'Access violation'.

peters avatar Oct 13 '13 10:10 peters

  1. You specify an invalid url (borked URI)
  2. The folder containing updates does not exist

Catching those errors and surfacing them as ShimmerConfigurationExceptions (and updating the sample to demonstrate those issues) is something I had started implementing (but put aside because of other priorities).

Now that someone else has hit the same issue, I feel much more confident about adding those in, for real.

shiftkey avatar Oct 13 '13 10:10 shiftkey

Oops, I'm not thinking of something I planned to do, but something I'd already shipped:

https://github.com/github/Shimmer/commit/d450bacbe37331b96466769952b4c310401a871f

Ok, so coming back to your code above - there's two things we could do in this case:

Throw an exception

Pros:

  • blindlingly obvious when things aren't right
  • exceptions can point users to documentation explaining configuration/setup

Cons:

  • everyone has to check for this (which kinda sucks)

Don't throw an exception

Pros:

  • simpler code for consumers

Cons:

  • "why doesn't my update work?" issues being raised (which could be offset by current logging messages)

I'm still leaning towards the first scenario (why the exception messages aren't being raised correctly is something I'll look into) - how would you expect this "configuration is wrong" message to appear to your application?

shiftkey avatar Oct 13 '13 10:10 shiftkey

Throw an exception blindlingly obvious when things aren't right

Exception based flow control is tha shit!

everyone has to check for this (which kinda sucks)

Let the exceptions buble -> e.g AggregateException

how would you expect this "configuration is wrong" message to appear to your application?

Hmm, that's a though one. Maybe something like this:

  1. InvalidUpdateUrlException (Maybe use default framework exception here?)
  2. UpdateFolderNotFoundException (could not establish connection, unable to list folder content)
  3. CheckForReleaseException (some internal state in shimmer prevents checking for updates)
  4. DownloadReleaseException (unable to download update)
  5. ApplyReleaseException (unable to apply release(s))

These exceptions should inherit from UpdateAggregateException so you only need to declare a catch clause around the super.

Find new names please, these are terrible :dancers:

peters avatar Oct 13 '13 11:10 peters

So I added a test like this:

[Fact]
public void WhenFolderDoesNotExistThrowHelpfulErrorAsync()
{
    string tempDir;
    using (Utility.WithTempDirectory(out tempDir))
    {
        var directory = Path.Combine(tempDir, "missing-folder");
        var fixture = new UpdateManager(directory, "MyAppName", FrameworkVersion.Net40);

        using (fixture)
        {
            AssertEx.TaskThrows<ShimmerConfigurationException>(
                async () => await fixture.CheckForUpdate());
        }
    }
}

Gives me this lovely error:

screen shot 2013-10-13 at 10 52 22 pm

I suspect I need to revisit the behaviour of the custom awaiter we're using here as there's too much wrapping of crap (basically).

Does that line up with what you're seeing in your testing?

shiftkey avatar Oct 13 '13 11:10 shiftkey

Does that line up with what you're seeing in your testing?

Yes, sir!

peters avatar Oct 13 '13 12:10 peters

Catching those errors and surfacing them as ShimmerConfigurationExceptions (and updating the sample to demonstrate those issues) is something I had started implementing (but put aside because of other priorities).

Cool, solves https://github.com/github/Shimmer/issues/201#issuecomment-26216405

peters avatar Oct 13 '13 12:10 peters

@peters ok great, that gives me more to go with

In terms of raising exceptions, these are my current thoughts on it:

Things which need to be addressed (either by the user or by the developers who wrote the application) should be raised and handled accordingly. If an update cannot be done (for whatever reason) it should not impact the behaviour of the application. Don't nag the user and don't crash the app - the user doesn't care, unless they're updating the application by hand.

InvalidUpdateUrlException (Maybe use default framework exception here?) UpdateFolderNotFoundException (could not establish connection, unable to list folder content)

I'd group these together with a helpful error message for each - they both occur within CheckForUpdates so having multiple catch-es feels redundant.

CheckForReleaseException (some internal state in shimmer prevents checking for updates)

We're now ensuring we return an UpdateInfo result always - I'm not sure what might come up here aside from the preconditions discussed above. Timeout are the big headache, I think we should just log those rather than interrupt the user. Same thing with connectivity (or lack of).

DownloadReleaseException (unable to download update)

Two things can come up here:

  • 404 (that's bad, and quite possibly a permanent issue)
  • Timeout (less bad, more likely to be transient)

I think the first one is worth interrupting the user for. The second one, not really.

ApplyReleaseException (unable to apply release(s))

This is where we get into the hairy stuff, things like:

  • the checksum validation fails (can't update! :( )
  • file corruption (maybe!)

These should interrupt, but how much can be resolved at this point is hard to say.

These exceptions should inherit from UpdateAggregateException so you only need to declare a catch clause around the super.

If you want to swallow the exceptions, just listen for Exception and react accordingly. If you want to catch specific exceptions, declare them before.

I've got background updating to implement for the desktop demo - I'll use async/await there as a way of fleshing out these bugs and scenarios.

shiftkey avatar Oct 13 '13 12:10 shiftkey

Two things can come up here: 404 (that's bad, and quite possibly a permanent issue) Timeout (less bad, more likely to be transient)

Yeah, this is pretty hairy stuff. I'm thinking maybe monotorrent could be fit for Shimmer (as an alternative). Obviously not everybody has the resources to host artifacts in the cloud (torrent metadata), but i'm seriously thinking about doing just so because deployments in the future might be in "hostile" environments where you never, ever, never, ever are going to get 100% of the bits you need.

I've used monotorrent in a project before, it's simple to use.

Probably post 1.0 though :hamster:

peters avatar Oct 13 '13 12:10 peters

I've got background updating to implement for the desktop demo - I'll use async/await there as a way of fleshing out these bugs and scenarios.

Nice!

peters avatar Oct 13 '13 12:10 peters

Obviously not everybody has the resources to host artifacts in the cloud (torrent metadata), but i'm seriously thinking about doing just so because deployments in the future might be in "hostile" environments where you never, ever, never, ever are going to get 100% of the bits you need.

We've been looking to use BITS to support more robust downloading of updates, but this is definitely a post-1.0 feature...

shiftkey avatar Oct 13 '13 21:10 shiftkey

@shiftkey Hmm, i'm trying to exclude two packages from my nuspec dependency list because these are being obsfucated by SmartAssembly, but using the following nuspec (the files are still being included):

<?xml version="1.0"?>
<package xmlns="http://schemas.microsoft.com/packaging/2010/07/nuspec.xsd">
  <metadata>
    ... bla bla bla ...
    <dependencies>
      <dependency id="AbbyyOnlineSdk" version="$version$" />
      <dependency id="AsposeCloud.SDK" version="$version$" />
      <dependency id="HigLabo" version="$version$" />
      <dependency id="Magick.NET-Q16-x86" version="$version$" />
      <dependency id="ModernUI" version="$version$" />
      <dependency id="NLog" version="$version$" />
      <dependency id="protobuf-net" version="$version$" />
      <dependency id="semver" version="$version$" />
      <dependency id="ServiceStack.Text" version="$version$" />
      <dependency id="Tesseract" version="$version$" />
      <dependency id="xps2img" />
     <!- two packages has been purposely left out here -->
    </dependencies>
  </metadata>
</package>

Any ideas on how i this can be achieved?

peters avatar Oct 13 '13 21:10 peters

Edit: The nupkg is generated correctly, but it's shimmer that references them (full pkg).

peters avatar Oct 13 '13 21:10 peters

Inside the packages.config specify developmentDependency="true" for whichever packages should be excluded.

shiftkey avatar Oct 13 '13 21:10 shiftkey

:+1: :+1: :+1: :+1: :+1:

peters avatar Oct 13 '13 22:10 peters

Right, so I'm now stuck between a rock and a hard place on this issue.

When you throw an exception after the fact (like we do when we await an AwaitableAsyncSubject), the stack trace is thrown away, like this:

screen shot 2013-10-14 at 12 23 02 pm

Without a really nice message (or stacktrace) these issues were almost impossible to resolve without a debugger attached. I wanted something better, which meant wrapping the exception to preserve the stacktrace.

Time to spend some time digging into CLR bits :trollface:

shiftkey avatar Oct 14 '13 01:10 shiftkey

@shiftkey In Shimmer 0.7.4 suddenly all project references (not nuget packages) are by default added to the nupkg and this was not the case before. Trying to override with <references /> but still has no effect. The reason this is important is that they need to be obsfucated. This could be solved with an AfterCompile target, but there's a bug in SmartAssembly so this does not work either. Ideas?

peters avatar Oct 26 '13 09:10 peters

Without a really nice message (or stacktrace) these issues were almost impossible to resolve without a debugger attached. I wanted something better, which meant wrapping the exception to preserve the stacktrace.

This is only fixed in .NET 4.5, if you're using 4.0 you're up a creek :(

anaisbetts avatar Oct 26 '13 18:10 anaisbetts

@paulcbetts I eventually moved the entire build process out of visual studio and ending up with creating the nupkg myself. Here's the code snippet i used for creating a squirrel release:

function MyGet-Squirrel-New-Release {
    param(
        [Parameter(Position = 0, Mandatory = $true, ValueFromPipeline = $true)]
        [string]$solutionFolder,
        [Parameter(Position = 1, Mandatory = $true, ValueFromPipeline = $true)]
        [string]$buildFolder
    )

    $packagesDir = Join-Path $solutionFolder "packages"

    $commandsPsm1 = MyGet-Grep -folder $packagesDir -recursive $true `
        -pattern "Shimmer.*commands.psm1$" | 
        Sort-Object FullName -Descending |
        Select-Object -first 1

    if(-not (Test-Path $commandsPsm1.FullName)) {
        MyGet-Die "Could not find any Squirrel nupkg's containing commands.psm1"
    }  

    MyGet-Write-Diagnostic "Squirrel: Creating new release"  

    Import-Module $commandsPsm1.FullName

    New-ReleaseForPackage -SolutionDir $solutionFolder -BuildDir $buildFolder

}

peters avatar Oct 26 '13 19:10 peters

In shimmer 0.7.4 automatic updates are still broken. I get the following error message when attempting to update one of my applications:

11/20/2013 11:42:37 Thread: AutoKollekt - Downloading 1 updates. 
11/20/2013 11:42:44 Thread: AutoKollekt - Applying 1 updates. 
11/20/2013 11:42:44 Thread: AutoKollekt - Error when updating application EXCEPTION OCCURRED:System.InvalidOperationException: AwaitableAsyncSubject<T>.GetResult() is rethrowing an inner exception ---> System.InvalidOperationException: AwaitableAsyncSubject<T>.GetResult() is rethrowing an inner exception ---> System.IO.FileLoadException: Could not load file or assembly 'NuGet.Core, Version=2.7.40808.167, Culture=neutral, PublicKeyToken=31bf3856ad364e35' or one of its dependencies. The located assembly's manifest definition does not match the assembly reference. (Exception from HRESULT: 0x80131040)
   at Shimmer.Client.UpdateManager.installPackageToAppDir(UpdateInfo updateInfo, ReleaseEntry release)
   at Shimmer.Client.UpdateManager.<>c__DisplayClass1d.<applyReleases>b__1a()
   at System.Reactive.Linq.QueryLanguage.<>c__DisplayClasscb`1.<>c__DisplayClasscd.<ToAsync>b__ca()
   --- End of inner exception stack trace ---
   at ReactiveUIMicro.AwaitableAsyncSubject`1.GetResult()
   at Shimmer.Client.UpdateManager.<applyReleases>d__1f.MoveNext()
   --- End of inner exception stack trace ---
   at ReactiveUIMicro.AwaitableAsyncSubject`1.GetResult()
   at odi.autokollekt.Config.AutoKollektApiHandler.<>c__DisplayClass2c.<<Maintenance>b__2a>d__2e.MoveNext()    at ReactiveUIMicro.AwaitableAsyncSubject`1.GetResult()
   at odi.autokollekt.Config.AutoKollektApiHandler.<>c__DisplayClass2c.<<Maintenance>b__2a>d__2e.MoveNext()

peters avatar Nov 20 '13 10:11 peters

Assembly manifest

<?xml version="1.0" encoding="utf-8"?>
<configuration>
    <startup> 
        <supportedRuntime version="v4.0" sku=".NETFramework,Version=v4.5" />
    </startup>
  <runtime>
    <assemblyBinding xmlns="urn:schemas-microsoft-com:asm.v1">
      <dependentAssembly>
        <assemblyIdentity name="System.Data.SQLite" publicKeyToken="db937bc2d44ff139" culture="neutral" />
        <bindingRedirect oldVersion="0.0.0.0-1.0.89.0" newVersion="1.0.89.0" />
      </dependentAssembly>
      <dependentAssembly>
        <assemblyIdentity name="Common.Logging" publicKeyToken="af08829b84f0328e" culture="neutral" />
        <bindingRedirect oldVersion="0.0.0.0-2.0.0.0" newVersion="2.0.0.0" />
      </dependentAssembly>
      <dependentAssembly>
        <assemblyIdentity name="System.Runtime" publicKeyToken="b03f5f7f11d50a3a" culture="neutral" />
        <bindingRedirect oldVersion="0.0.0.0-4.0.0.0" newVersion="4.0.0.0" />
      </dependentAssembly>
      <dependentAssembly>
        <assemblyIdentity name="System.Threading.Tasks" publicKeyToken="b03f5f7f11d50a3a" culture="neutral" />
        <bindingRedirect oldVersion="0.0.0.0-4.0.0.0" newVersion="4.0.0.0" />
      </dependentAssembly>
      <dependentAssembly>
        <assemblyIdentity name="System.IO.Abstractions" publicKeyToken="d480b5b72fb413da" culture="neutral" />
        <bindingRedirect oldVersion="0.0.0.0-1.4.0.68" newVersion="1.4.0.68" />
      </dependentAssembly>
      <dependentAssembly>
        <assemblyIdentity name="NuGet.Core" publicKeyToken="31bf3856ad364e35" culture="neutral" />
        <bindingRedirect oldVersion="0.0.0.0-2.7.40911.225" newVersion="2.7.40911.225" />
      </dependentAssembly>
    </assemblyBinding>
  </runtime>
</configuration>

peters avatar Nov 20 '13 10:11 peters