Skip to content
This repository has been archived by the owner on Jan 23, 2023. It is now read-only.

port changes in SDK after Microsoft.NET.HostModel.AppHost is created #7373

Merged
merged 2 commits into from
Jul 24, 2019
Merged

port changes in SDK after Microsoft.NET.HostModel.AppHost is created #7373

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
0059721
Fix P/Invoke signatures for the resource updater.
Jul 24, 2019
7f0eb28
Delete output apphost upon failure of the CreateAppHost task.
Jul 24, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 21 additions & 0 deletions src/managed/Microsoft.NET.HostModel/AppHost/FailedToDeleteApphostException.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
// Licensed to the .NET Foundation under one or more agreements.
// The .NET Foundation licenses this file to you under the MIT license.
// See the LICENSE file in the project root for more information.

using System;

namespace Microsoft.NET.HostModel.AppHost
{
/// <summary>
/// Failed to delete apphost when trying to delete incomplete appphost
/// </summary>
public class FailedToDeleteApphostException : AppHostUpdateException
{
public string ExceptionMessage { get; }
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why isn't this passed to base so that we get it as Exception.Message?

public FailedToDeleteApphostException(string exceptionMessage)
{
ExceptionMessage = exceptionMessage;
}
}
}

70 changes: 47 additions & 23 deletions src/managed/Microsoft.NET.HostModel/AppHost/HostWriter.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
// Licensed under the MIT license. See LICENSE file in the project root for full license information.

using System;
using System.Diagnostics;
using System.IO;
using System.IO.MemoryMappedFiles;
using System.Text;
Expand Down Expand Up @@ -43,45 +44,68 @@ public static void CreateAppHost(

BinaryUtils.CopyFile(appHostSourceFilePath, appHostDestinationFilePath);

// Re-write the destination apphost with the proper contents.
bool appHostIsPEImage = false;
using (var memoryMappedFile = MemoryMappedFile.CreateFromFile(appHostDestinationFilePath))
try
{
using (MemoryMappedViewAccessor accessor = memoryMappedFile.CreateViewAccessor())
// Re-write the destination apphost with the proper contents.
bool appHostIsPEImage = false;
using (var memoryMappedFile = MemoryMappedFile.CreateFromFile(appHostDestinationFilePath))
{
BinaryUtils.SearchAndReplace(accessor, AppBinaryPathPlaceholderSearchValue, bytesToWrite);
using (MemoryMappedViewAccessor accessor = memoryMappedFile.CreateViewAccessor())
{
BinaryUtils.SearchAndReplace(accessor, AppBinaryPathPlaceholderSearchValue, bytesToWrite);

appHostIsPEImage = BinaryUtils.IsPEImage(accessor);
appHostIsPEImage = BinaryUtils.IsPEImage(accessor);

if (windowsGraphicalUserInterface)
{
if (!appHostIsPEImage)
if (windowsGraphicalUserInterface)
{
throw new AppHostNotPEFileException();
if (!appHostIsPEImage)
{
throw new AppHostNotPEFileException();
}

BinaryUtils.SetWindowsGraphicalUserInterfaceBit(accessor);
}
}
}

BinaryUtils.SetWindowsGraphicalUserInterfaceBit(accessor);
if (assemblyToCopyResorcesFrom != null && appHostIsPEImage)
{
if (ResourceUpdater.IsSupportedOS())
{
// Copy resources from managed dll to the apphost
new ResourceUpdater(appHostDestinationFilePath)
.AddResourcesFromPEImage(assemblyToCopyResorcesFrom)
.Update();
}
else
{
throw new AppHostCustomizationUnsupportedOSException();
}
}
}

if (assemblyToCopyResorcesFrom != null && appHostIsPEImage)
// Memory-mapped write does not updating last write time
File.SetLastWriteTimeUtc(appHostDestinationFilePath, DateTime.UtcNow);
}
catch (Exception ex)
{
if (ResourceUpdater.IsSupportedOS())
FailedToDeleteApphostException failedToDeleteApphostException = null;
// Delete the destination file so we don't leave an unmodified apphost
try
{
// Copy resources from managed dll to the apphost
new ResourceUpdater(appHostDestinationFilePath)
.AddResourcesFromPEImage(assemblyToCopyResorcesFrom)
.Update();
File.Delete(appHostDestinationFilePath);
}
else
catch (Exception failedToDeleteEx) when (failedToDeleteEx is IOException || failedToDeleteEx is UnauthorizedAccessException)
wli3 marked this conversation as resolved.
Show resolved Hide resolved
{
throw new AppHostCustomizationUnsupportedOSException();
failedToDeleteApphostException = new FailedToDeleteApphostException(failedToDeleteEx.Message);
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would put failedToDeleteEx in innner exception so that the AggregateException has all stack traces.

Copy link

@nguerrera nguerrera Jul 24, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, do we need FailedToDeleteException at all? Can we just throw an AggregateException of the deletion exception and the original exception?

The goal was not to lose the info if Delete is also failing, but an arbitrary exception here is going to become a big scary stack trace in MSBuild, right? If we aggregate both, we'll have all the info in that bug report. The SDK wouldn't need to handle AggregateException specially and log it.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would also remove the when from the Delete catch.

try
{
    // create apphost
}
catch (Exception ex)
{
     try
     {
            // delete apphost
     }
     catch (Exception deleteEx)
     {
            throw new AggregateException(ex, deleteEx);
     }

     throw;
}

}
}

// Memory-mapped write does not updating last write time
File.SetLastWriteTimeUtc(appHostDestinationFilePath, DateTime.UtcNow);
if (failedToDeleteApphostException != null)
{
throw new AggregateException(ex, failedToDeleteApphostException);
}

throw;
}
}

/// <summary>
Expand Down
4 changes: 2 additions & 2 deletions src/managed/Microsoft.NET.HostModel/AppHost/ResourceUpdater.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@ private sealed class Kernel32
// Native methods for updating resources
//

[DllImport(nameof(Kernel32), SetLastError=true)]
[DllImport(nameof(Kernel32), CharSet = CharSet.Unicode, SetLastError=true)]
public static extern SafeUpdateHandle BeginUpdateResource(string pFileName,
[MarshalAs(UnmanagedType.Bool)]bool bDeleteExistingResources);

Expand Down Expand Up @@ -71,7 +71,7 @@ public enum LoadLibraryFlags : uint
LOAD_LIBRARY_AS_IMAGE_RESOURCE = 0x00000020
}

[DllImport(nameof(Kernel32), SetLastError=true)]
[DllImport(nameof(Kernel32), CharSet = CharSet.Unicode, SetLastError=true)]
public static extern IntPtr LoadLibraryEx(string lpFileName,
IntPtr hReservedNull,
LoadLibraryFlags dwFlags);
Expand Down