Skip to content

Commit

Permalink
Address more code review feedback:
Browse files Browse the repository at this point in the history 
* Fix Assembly.ni.dll and Assembly.dll path usage.
* Remove unused full-path computation for corelib when loading from bundle.
* Add tracing when loading from bundle.
* Add probing for satellite assemblies.
* Fix test to not wait infinitely for synchronization.
  • Loading branch information
@swaroop-sridhar
swaroop-sridhar committed May 22, 2020
1 parent 10732fe commit 7cc1384
Show file tree
Hide file tree
Showing 4 changed files with 180 additions and 83 deletions.
251 changes: 172 additions & 79 deletions src/coreclr/src/binder/assemblybinder.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -381,27 +381,34 @@ namespace BINDER_SPACE

ReleaseHolder<Assembly> pSystemAssembly;

StackSString sCoreLibDir(systemDirectory);
if (!sCoreLibDir.EndsWith(DIRECTORY_SEPARATOR_CHAR_W))
{
sCoreLibDir.Append(DIRECTORY_SEPARATOR_CHAR_W);
}

StackSString sCoreLib(sCoreLibDir);
StackSString coreLibName(CoreLibName_IL_W);
sCoreLib.Append(coreLibName);

// At run-time, System.Private.CoreLib.dll is expected to be the NI image.
// System.Private.CoreLib.dll is expected to be found at one of the following locations:
// * Non-single-file app: In systemDirectory, beside coreclr.dll
// * Framework-dependent single-file app: In systemDirectory, beside coreclr.dll
// * Self-contained single-file app: Within the single-file bundle.
//
// CoreLib path (sCoreLib):
// * Absolute path when looking for a file on disk
// * Bundle-relative path when looking within the single-file bundle.

StackSString sCoreLibName(CoreLibName_IL_W);
StackSString sCoreLib;
BinderTracing::PathSource pathSource = BinderTracing::PathSource::Bundle;
BundleFileLocation bundleFileLocation = Bundle::ProbeAppBundle(sCoreLibName, /*pathIsBundleRelative */ true);
if (!bundleFileLocation.IsValid())
{
sCoreLib.Set(systemDirectory);
pathSource = BinderTracing::PathSource::ApplicationAssemblies;
}
CombinePath(sCoreLib, sCoreLibName, sCoreLib);

IF_FAIL_GO(AssemblyBinder::GetAssembly(sCoreLib,
TRUE /* fIsInGAC */,
fBindToNativeImage,
&pSystemAssembly,
NULL /* szMDAssemblyPath */,
Bundle::ProbeAppBundle(coreLibName, /*pathIsBundleRelative */ true)));
TRUE /* fIsInGAC */,
fBindToNativeImage,
&pSystemAssembly,
NULL /* szMDAssemblyPath */,
bundleFileLocation));
BinderTracing::PathProbed(sCoreLib, pathSource, hr);

*ppSystemAssembly = pSystemAssembly.Extract();

Expand All @@ -411,10 +418,10 @@ namespace BINDER_SPACE


/* static */
HRESULT AssemblyBinder::BindToSystemSatellite(SString &systemDirectory,
SString &simpleName,
SString &cultureName,
Assembly **ppSystemAssembly)
HRESULT AssemblyBinder::BindToSystemSatellite(SString& systemDirectory,
SString& simpleName,
SString& cultureName,
Assembly** ppSystemAssembly)
{
// Indirect check that binder was initialized.
_ASSERTE(g_BinderVariables != NULL);
Expand All @@ -438,8 +445,18 @@ namespace BINDER_SPACE
// append extension
relativePath.Append(W(".dll"));

// Satellite assembly's absolute path
StackSString sMscorlibSatellite(systemDirectory);
// Satellite assembly's path:
// * Absolute path when looking for a file on disk
// * Bundle-relative path when looking within the single-file bundle.
StackSString sMscorlibSatellite;

BinderTracing::PathSource pathSource = BinderTracing::PathSource::Bundle;
BundleFileLocation bundleFileLocation = Bundle::ProbeAppBundle(relativePath, /*pathIsBundleRelative */ true);
if (!bundleFileLocation.IsValid())
{
sMscorlibSatellite.Set(systemDirectory);
pathSource = BinderTracing::PathSource::ApplicationAssemblies;
}
CombinePath(sMscorlibSatellite, relativePath, sMscorlibSatellite);

ReleaseHolder<Assembly> pSystemAssembly;
Expand All @@ -448,7 +465,8 @@ namespace BINDER_SPACE
FALSE /* fExplicitBindToNativeImage */,
&pSystemAssembly,
NULL /* szMDAssemblyPath */,
Bundle::ProbeAppBundle(relativePath, /*pathIsBundleRelative */ true)));
bundleFileLocation));
BinderTracing::PathProbed(sMscorlibSatellite, pathSource, hr);

*ppSystemAssembly = pSystemAssembly.Extract();

Expand Down Expand Up @@ -735,36 +753,75 @@ namespace BINDER_SPACE

namespace
{
typedef void (*OnPathProbed)(const WCHAR *path, HRESULT hr);

HRESULT BindSatelliteResourceByProbingPaths(
const StringArrayList *pResourceRoots,
AssemblyName *pRequestedAssemblyName,
BindResult *pBindResult,
OnPathProbed onPathProbed)
HRESULT BindSatelliteResourceFromBundle(
AssemblyName* pRequestedAssemblyName,
SString &relativePath,
BindResult* pBindResult)
{
HRESULT hr = S_OK;

SString &simpleNameRef = pRequestedAssemblyName->GetSimpleName();
SString &cultureRef = pRequestedAssemblyName->GetCulture();
BundleFileLocation bundleFileLocation = Bundle::ProbeAppBundle(relativePath, /* pathIsBundleRelative */ true);
if (!bundleFileLocation.IsValid())
{
return hr;
}

_ASSERTE(!cultureRef.IsEmpty() && !cultureRef.EqualsCaseInsensitive(g_BinderVariables->cultureNeutral));
ReleaseHolder<Assembly> pAssembly;
hr = AssemblyBinder::GetAssembly(relativePath,
FALSE /* fIsInGAC */,
FALSE /* fExplicitBindToNativeImage */,
&pAssembly,
NULL, // szMDAssemblyPath
bundleFileLocation);

BinderTracing::PathProbed(relativePath, BinderTracing::PathSource::Bundle, hr);

// Missing files are okay and expected when probing
if (hr == HRESULT_FROM_WIN32(ERROR_FILE_NOT_FOUND))
{
return S_OK;
}

pBindResult->SetAttemptResult(hr, pAssembly);
if (FAILED(hr))
return hr;

AssemblyName* pBoundAssemblyName = pAssembly->GetAssemblyName();
if (TestCandidateRefMatchesDef(pRequestedAssemblyName, pBoundAssemblyName, false /*tpaListAssembly*/))
{
pBindResult->SetResult(pAssembly);
hr = S_OK;
}
else
{
hr = FUSION_E_REF_DEF_MISMATCH;
}

pBindResult->SetAttemptResult(hr, pAssembly);
return hr;
}

HRESULT BindSatelliteResourceByProbingPaths(
const StringArrayList *pResourceRoots,
AssemblyName *pRequestedAssemblyName,
SString &relativePath,
BindResult *pBindResult,
BinderTracing::PathSource pathSource)
{
HRESULT hr = S_OK;

for (UINT i = 0; i < pResourceRoots->GetCount(); i++)
{
ReleaseHolder<Assembly> pAssembly;
SString &wszBindingPath = (*pResourceRoots)[i];
SString fileName(wszBindingPath);

CombinePath(fileName, cultureRef, fileName);
CombinePath(fileName, simpleNameRef, fileName);
fileName.Append(W(".dll"));
CombinePath(fileName, relativePath, fileName);

hr = AssemblyBinder::GetAssembly(fileName,
FALSE /* fIsInGAC */,
FALSE /* fExplicitBindToNativeImage */,
&pAssembly);
onPathProbed(fileName, hr);
BinderTracing::PathProbed(relativePath, pathSource, hr);

// Missing files are okay and expected when probing
if (hr == HRESULT_FROM_WIN32(ERROR_FILE_NOT_FOUND))
Expand Down Expand Up @@ -795,14 +852,66 @@ namespace BINDER_SPACE
return S_OK;
}

HRESULT BindSatelliteResource(
ApplicationContext* pApplicationContext,
AssemblyName* pRequestedAssemblyName,
BindResult* pBindResult)
{
// Satellite resource probing strategy is to look:
// * First within the single-file bundle
// * Then under each of the Platform Resource Roots
// * Then under each of the App Paths.
//
// During each search, if we find a platform resource file with matching file name, but whose ref-def didn't match,
// fall back to application resource lookup to handle case where a user creates resources with the same
// names as platform ones.

HRESULT hr = S_OK;
SString& simpleNameRef = pRequestedAssemblyName->GetSimpleName();
SString& cultureRef = pRequestedAssemblyName->GetCulture();

_ASSERTE(!cultureRef.IsEmpty() && !cultureRef.EqualsCaseInsensitive(g_BinderVariables->cultureNeutral));

ReleaseHolder<Assembly> pAssembly;
SString fileName;
CombinePath(fileName, cultureRef, fileName);
CombinePath(fileName, simpleNameRef, fileName);
fileName.Append(W(".dll"));

hr = BindSatelliteResourceFromBundle(pRequestedAssemblyName, fileName, pBindResult);

if (pBindResult->HaveResult() || (FAILED(hr) && hr != FUSION_E_CONFIGURATION_ERROR))
{
return hr;
}

hr = BindSatelliteResourceByProbingPaths(pApplicationContext->GetPlatformResourceRoots(),
pRequestedAssemblyName,
fileName,
pBindResult,
BinderTracing::PathSource::PlatformResourceRoots);

if (pBindResult->HaveResult() || (FAILED(hr) && hr != FUSION_E_CONFIGURATION_ERROR))
{
return hr;
}

hr = BindSatelliteResourceByProbingPaths(pApplicationContext->GetAppPaths(),
pRequestedAssemblyName,
fileName,
pBindResult,
BinderTracing::PathSource::AppPaths);

return hr;
}

HRESULT BindAssemblyByProbingPaths(
const StringArrayList *pBindingPaths,
AssemblyName *pRequestedAssemblyName,
bool useNativeImages,
Assembly **ppAssembly)
{
SString &simpleName = pRequestedAssemblyName->GetSimpleName();

BinderTracing::PathSource pathSource = useNativeImages ? BinderTracing::PathSource::AppNativeImagePaths : BinderTracing::PathSource::AppPaths;
// Loop through the binding paths looking for a matching assembly
for (DWORD i = 0; i < pBindingPaths->GetCount(); i++)
Expand Down Expand Up @@ -896,31 +1005,7 @@ namespace BINDER_SPACE

if (!culture.IsEmpty() && !culture.EqualsCaseInsensitive(g_BinderVariables->cultureNeutral))
{
//
// Satellite resource probing strategy is to look under each of the Platform Resource Roots
// followed by App Paths.
//

hr = BindSatelliteResourceByProbingPaths(pApplicationContext->GetPlatformResourceRoots(),
pRequestedAssemblyName,
pBindResult,
[](const WCHAR *path, HRESULT res) { BinderTracing::PathProbed(path, BinderTracing::PathSource::PlatformResourceRoots, res); });

// We found a platform resource file with matching file name, but whose ref-def didn't match. Fall
// back to application resource lookup to handle case where a user creates resources with the same
// names as platform ones.
if (hr != FUSION_E_CONFIGURATION_ERROR)
{
IF_FAIL_GO(hr);
}

if (!pBindResult->HaveResult())
{
IF_FAIL_GO(BindSatelliteResourceByProbingPaths(pApplicationContext->GetAppPaths(),
pRequestedAssemblyName,
pBindResult,
[](const WCHAR *path, HRESULT res) { BinderTracing::PathProbed(path, BinderTracing::PathSource::AppPaths, res); }));
}
IF_FAIL_GO(BindSatelliteResource(pApplicationContext, pRequestedAssemblyName, pBindResult));
}
else
{
Expand All @@ -934,7 +1019,9 @@ namespace BINDER_SPACE
// If found, the assembly is loaded from the bundle.
if (Bundle::AppIsBundle())
{
SString candidates[] = { W(".dll"), W(".ni.dll") };
// Search Assembly.ni.dll, then Assembly.dll
// The Assembly.ni.dll paths are rare, and intended for supporting managed C++ R2R assemblies.
SString candidates[] = { W(".ni.dll"), W(".dll") };

// Loop through the binding paths looking for a matching assembly
for (int i = 0; i < 2; i++)
Expand All @@ -945,24 +1032,30 @@ namespace BINDER_SPACE
SString assemblyFilePath(Bundle::AppBundle->BasePath());
assemblyFilePath.Append(assemblyFileName);

hr = GetAssembly(assemblyFilePath,
TRUE, // fIsInGAC
FALSE, // fExplicitBindToNativeImage
&pTPAAssembly,
NULL, // szMDAssemblyPath
Bundle::ProbeAppBundle(assemblyFileName, /* pathIsBundleRelative */ true));

if (hr != HRESULT_FROM_WIN32(ERROR_FILE_NOT_FOUND))
BundleFileLocation bundleFileLocation = Bundle::ProbeAppBundle(assemblyFileName, /* pathIsBundleRelative */ true);
if (bundleFileLocation.IsValid())
{
// Any other error is fatal
IF_FAIL_GO(hr);
hr = GetAssembly(assemblyFilePath,
TRUE, // fIsInGAC
FALSE, // fExplicitBindToNativeImage
&pTPAAssembly,
NULL, // szMDAssemblyPath
bundleFileLocation);

BinderTracing::PathProbed(assemblyFilePath, BinderTracing::PathSource::Bundle, hr);

if (TestCandidateRefMatchesDef(pRequestedAssemblyName, pTPAAssembly->GetAssemblyName(), true /*tpaListAssembly*/))
if (hr != HRESULT_FROM_WIN32(ERROR_FILE_NOT_FOUND))
{
// We have found the requested assembly match in the bundle with validation of the full-qualified name.
// Bind to it.
pBindResult->SetResult(pTPAAssembly);
GO_WITH_HRESULT(S_OK);
// Any other error is fatal
IF_FAIL_GO(hr);

if (TestCandidateRefMatchesDef(pRequestedAssemblyName, pTPAAssembly->GetAssemblyName(), true /*tpaListAssembly*/))
{
// We have found the requested assembly match in the bundle with validation of the full-qualified name.
// Bind to it.
pBindResult->SetResult(pTPAAssembly);
GO_WITH_HRESULT(S_OK);
}
}
}
}
Expand Down
3 changes: 2 additions & 1 deletion src/coreclr/src/binder/inc/bindertracing.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -180,7 +180,8 @@ namespace BinderTracing
AppNativeImagePaths,
AppPaths,
PlatformResourceRoots,
SatelliteSubdirectory
SatelliteSubdirectory,
Bundle
};

void PathProbed(const WCHAR *path, PathSource source, HRESULT hr);
Expand Down
2 changes: 1 addition & 1 deletion src/coreclr/src/binder/utils.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -80,7 +80,7 @@ namespace BINDER_SPACE
SString platformPathSeparator(SString::Literal, GetPlatformPathSeparator());
combinedPath.Set(pathA);

if (!combinedPath.EndsWith(platformPathSeparator))
if (!combinedPath.IsEmpty() && !combinedPath.EndsWith(platformPathSeparator))
{
combinedPath.Append(platformPathSeparator);
}
Expand Down
7 changes: 5 additions & 2 deletions src/installer/test/Microsoft.NET.HostModel.Tests/AppHost.Bundle.Tests/BundleRename.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -51,17 +51,20 @@ private void Bundle_can_be_renamed_while_running(bool renameFirstRun)
.CaptureStdOut()
.Start();

while (!File.Exists(waitFile) && !singleExe.Process.HasExited)
const int twoMitutes = 120000 /*milliseconds*/;
int waitTime = 0;
while (!File.Exists(waitFile) && !singleExe.Process.HasExited && waitTime < twoMitutes)
{
Thread.Sleep(100);
waitTime += 100;
}

Assert.True(File.Exists(waitFile));

File.Move(singleFile, renameFile);
File.Create(resumeFile).Close();

var result = singleExe.WaitForExit(fExpectedToFail: false);
var result = singleExe.WaitForExit(fExpectedToFail: false, twoMitutes);

result
.Should()
Expand Down

0 comments on commit 7cc1384

Please sign in to comment.