From e7b4696856650aa051e2e882dfc22d1e83d02657 Mon Sep 17 00:00:00 2001 From: Vitek Karas Date: Tue, 4 Dec 2018 15:26:52 -0800 Subject: [PATCH] Add new export to hostpolicy and hostfxr to redirect error output (dotnet/core-setup#4816) * Add new export to hostpolicy and hostfxr to redirect error output to a callback This introduces corehost_set_error_writer and hostfxr_set_error_writer exports. If set, all errors will be written to the error writer instead of the default stderr. Tracing is unaffected by this change. The error writer is set per-thread (thread local). Only one error writer can be set on a given thread. Subsequent calls to set error writer will overwrite the previous writer. hostfxr propagates the custom error writer (if any) to the hostpolicy for the duration of the calls it makes to hostpolicy. Added tests to validate the new behavior. * Remove TODOs which are now resolved with this change * Fix buffer allocation to work with any string (arbitrary large). Remove locks on error writer since it's thread local. * Fix the error formating on Linux * Fix error formatting on Linux va_list is not reusable, by definition. On Windows this works since the implementation makes it reusable. but on Linux it's not. So make a copy before calling the printf with it. Simplify the code, since vsnprintf and _vsnwprintf do in fact behave the same if passed NULL buffer and zero length (calculates the necessary buffer size). So no need for two functions even on Windows. Commit migrated from https://github.com/dotnet/core-setup/commit/bfb18700ea2fb0e600a034ac7f719aee2618a87e --- .../host-component-dependencies-resolution.md | 1 - src/installer/corehost/cli/fxr/hostfxr.cpp | 113 ++++++++++++++--- src/installer/corehost/cli/hostpolicy.cpp | 34 +++-- src/installer/corehost/common/pal.h | 3 + src/installer/corehost/common/trace.cpp | 31 ++++- src/installer/corehost/common/trace.h | 12 ++ .../TestProjects/HostApiInvokerApp/HostFXR.cs | 114 ++++++++++++++++- .../TestProjects/HostApiInvokerApp/HostPolicy.cs | 140 ++++++++++++++++++--- ...nThatICareAboutComponentDependencyResolution.cs | 80 ++++++++++-- .../GivenThatICareAboutNativeHostApis.cs | 77 ++++++++++++ 10 files changed, 550 insertions(+), 55 deletions(-) diff --git a/docs/installer/design-docs/host-component-dependencies-resolution.md b/docs/installer/design-docs/host-component-dependencies-resolution.md index 7307f17..094f3d0 100644 --- a/docs/installer/design-docs/host-component-dependencies-resolution.md +++ b/docs/installer/design-docs/host-component-dependencies-resolution.md @@ -38,7 +38,6 @@ This feature certainly provides a somewhat duplicate functionality to the existi * Review the list of settings which are reused from the app (see above) * Review the list of environment variables - if we should use the same as the app or not * Currently we don't consider frameworks for the app when computing probing paths for resolving assets from the component's `.deps.json`. This is a different behavior from the app startup where these are considered. Is it important - needed? -* Error reporting: If the native code fails currently it only returns error code. So next to no information. It writes detailed error message into the stderr (even with tracing disabled) which is also wrong since it pollutes the process output. It should be captured and returned to the caller, so that the managed code can include it in the exception. There's also consideration for localization - as currently the native components don't use localized error reporting. * Add ability to corelate tracing with the runtime - probably some kind of activity ID * Handling of native assets - currently returning just probing paths. Would be cleaner to return full resolved paths. But we would have to keep some probing paths. In the case of missing `.deps.json` the native library should be looked for in the component directory - thus requires probing - we can't figure out which of the files in the folder are native libraries in the hosts. * Handling of satellite assemblies (resource assets) - currently returning just probing paths which exclude the culture. So from a resolved asset `./foo/en-us/resource.dll` we only take `./foo` as the probing path. Consider using full paths instead - probably would require more parsing as we would have to be able to figure out the culture ID somewhere to build the true map AssemblyName->path in the managed class. Just like for native assets, if there's no `.deps.json` the only possible solution is to use probing, so the probing semantics would have to be supported anyway. \ No newline at end of file diff --git a/src/installer/corehost/cli/fxr/hostfxr.cpp b/src/installer/corehost/cli/fxr/hostfxr.cpp index 10a8dba..4404e9e 100644 --- a/src/installer/corehost/cli/fxr/hostfxr.cpp +++ b/src/installer/corehost/cli/fxr/hostfxr.cpp @@ -17,13 +17,16 @@ typedef int(*corehost_load_fn) (const host_interface_t* init); typedef int(*corehost_main_fn) (const int argc, const pal::char_t* argv[]); typedef int(*corehost_main_with_output_buffer_fn) (const int argc, const pal::char_t* argv[], pal::char_t buffer[], int32_t buffer_size, int32_t* required_buffer_size); typedef int(*corehost_unload_fn) (); +typedef void(*corehost_error_writer_fn) (const pal::char_t* message); +typedef corehost_error_writer_fn(*corehost_set_error_writer_fn) (corehost_error_writer_fn error_writer); int load_host_library_common( const pal::string_t& lib_dir, pal::string_t& host_path, pal::dll_t* h_host, corehost_load_fn* load_fn, - corehost_unload_fn* unload_fn) + corehost_unload_fn* unload_fn, + corehost_set_error_writer_fn* set_error_writer_fn) { if (!library_exists_in_dir(lib_dir, LIBHOSTPOLICY_NAME, &host_path)) { @@ -40,6 +43,11 @@ int load_host_library_common( // Obtain entrypoint symbols *load_fn = (corehost_load_fn)pal::get_symbol(*h_host, "corehost_load"); *unload_fn = (corehost_unload_fn)pal::get_symbol(*h_host, "corehost_unload"); + *set_error_writer_fn = (corehost_set_error_writer_fn)pal::get_symbol(*h_host, "corehost_set_error_writer"); + + // It's possible to not have corehost_set_error_writer, since this was only introduced in 3.0 + // so 2.0 hostpolicy would not have the export. In this case we will not propagate the error writer + // and errors will still be reported to stderr. return (*load_fn != nullptr) && (*unload_fn != nullptr) ? StatusCode::Success @@ -51,10 +59,11 @@ int load_host_library( pal::dll_t* h_host, corehost_load_fn* load_fn, corehost_main_fn* main_fn, - corehost_unload_fn* unload_fn) + corehost_unload_fn* unload_fn, + corehost_set_error_writer_fn* set_error_writer_fn) { pal::string_t host_path; - int rc = load_host_library_common(lib_dir, host_path, h_host, load_fn, unload_fn); + int rc = load_host_library_common(lib_dir, host_path, h_host, load_fn, unload_fn, set_error_writer_fn); if (rc != StatusCode::Success) { return rc; @@ -73,10 +82,11 @@ int load_host_library_with_return( pal::dll_t* h_host, corehost_load_fn* load_fn, corehost_main_with_output_buffer_fn* main_fn, - corehost_unload_fn* unload_fn) + corehost_unload_fn* unload_fn, + corehost_set_error_writer_fn* set_error_writer_fn) { pal::string_t host_path; - int rc = load_host_library_common(lib_dir, host_path, h_host, load_fn, unload_fn); + int rc = load_host_library_common(lib_dir, host_path, h_host, load_fn, unload_fn, set_error_writer_fn); if (rc != StatusCode::Success) { return rc; @@ -90,6 +100,37 @@ int load_host_library_with_return( : StatusCode::CoreHostEntryPointFailure; } +// Helper class to make it easy to propagate error writer to the hostpolicy +class propagate_error_writer_to_corehost_t +{ +private: + corehost_set_error_writer_fn m_set_error_writer_fn; + bool m_error_writer_set; + +public: + propagate_error_writer_to_corehost_t(corehost_set_error_writer_fn set_error_writer_fn) + { + m_set_error_writer_fn = set_error_writer_fn; + m_error_writer_set = false; + + trace::error_writer_fn error_writer = trace::get_error_writer(); + if (error_writer != nullptr && m_set_error_writer_fn != nullptr) + { + m_set_error_writer_fn(error_writer); + m_error_writer_set = true; + } + } + + ~propagate_error_writer_to_corehost_t() + { + if (m_error_writer_set && m_set_error_writer_fn != nullptr) + { + m_set_error_writer_fn(nullptr); + m_error_writer_set = false; + } + } +}; + int execute_app( const pal::string_t& impl_dll_dir, corehost_init_t* init, @@ -100,8 +141,9 @@ int execute_app( corehost_main_fn host_main = nullptr; corehost_load_fn host_load = nullptr; corehost_unload_fn host_unload = nullptr; + corehost_set_error_writer_fn host_set_error_writer = nullptr; - int code = load_host_library(impl_dll_dir, &corehost, &host_load, &host_main, &host_unload); + int code = load_host_library(impl_dll_dir, &corehost, &host_load, &host_main, &host_unload, &host_set_error_writer); if (code != StatusCode::Success) { trace::error(_X("An error occurred while loading required library %s from [%s]"), LIBHOSTPOLICY_NAME, impl_dll_dir.c_str()); @@ -111,11 +153,15 @@ int execute_app( // Previous hostfxr trace messages must be printed before calling trace::setup in hostpolicy trace::flush(); - const host_interface_t& intf = init->get_host_init_data(); - if ((code = host_load(&intf)) == 0) { - code = host_main(argc, argv); - (void)host_unload(); + propagate_error_writer_to_corehost_t propagate_error_writer_to_corehost(host_set_error_writer); + + const host_interface_t& intf = init->get_host_init_data(); + if ((code = host_load(&intf)) == 0) + { + code = host_main(argc, argv); + (void)host_unload(); + } } pal::unload_library(corehost); @@ -136,8 +182,9 @@ int execute_host_command( corehost_main_with_output_buffer_fn host_main = nullptr; corehost_load_fn host_load = nullptr; corehost_unload_fn host_unload = nullptr; + corehost_set_error_writer_fn host_set_error_writer = nullptr; - int code = load_host_library_with_return(impl_dll_dir, &corehost, &host_load, &host_main, &host_unload); + int code = load_host_library_with_return(impl_dll_dir, &corehost, &host_load, &host_main, &host_unload, &host_set_error_writer); if (code != StatusCode::Success) { @@ -148,11 +195,15 @@ int execute_host_command( // Previous hostfxr trace messages must be printed before calling trace::setup in hostpolicy trace::flush(); - const host_interface_t& intf = init->get_host_init_data(); - if ((code = host_load(&intf)) == 0) { - code = host_main(argc, argv, result_buffer, buffer_size, required_buffer_size); - (void)host_unload(); + propagate_error_writer_to_corehost_t propagate_error_writer_to_corehost(host_set_error_writer); + + const host_interface_t& intf = init->get_host_init_data(); + if ((code = host_load(&intf)) == 0) + { + code = host_main(argc, argv, result_buffer, buffer_size, required_buffer_size); + (void)host_unload(); + } } pal::unload_library(corehost); @@ -514,3 +565,35 @@ SHARED_API int32_t hostfxr_get_native_search_directories(const int argc, const p int rc = muxer.execute(_X("get-native-search-directories"), argc, argv, startup_info, buffer, buffer_size, required_buffer_size); return rc; } + + + +typedef void(*hostfxr_error_writer_fn)(const pal::char_t* message); + +// +// Sets a callback which is to be used to write errors to. +// +// Parameters: +// error_writer +// A callback function which will be invoked every time an error is to be reported. +// Or nullptr to unregister previously registered callback and return to the default behavior. +// Return value: +// The previously registered callback (which is now unregistered), or nullptr if no previous callback +// was registered +// +// The error writer is registered per-thread, so the registration is thread-local. On each thread +// only one callback can be registered. Subsequent registrations overwrite the previous ones. +// +// By default no callback is registered in which case the errors are written to stderr. +// +// Each call to the error writer is sort of like writing a single line (the EOL character is omitted). +// Multiple calls to the error writer may occure for one failure. +// +// If the hostfxr invokes functions in hostpolicy as part of its operation, the error writer +// will be propagated to hostpolicy for the duration of the call. This means that errors from +// both hostfxr and hostpolicy will be reporter through the same error writer. +// +SHARED_API hostfxr_error_writer_fn hostfxr_set_error_writer(hostfxr_error_writer_fn error_writer) +{ + return trace::set_error_writer(error_writer); +} diff --git a/src/installer/corehost/cli/hostpolicy.cpp b/src/installer/corehost/cli/hostpolicy.cpp index 51a9e28..11a9174 100644 --- a/src/installer/corehost/cli/hostpolicy.cpp +++ b/src/installer/corehost/cli/hostpolicy.cpp @@ -477,13 +477,6 @@ SHARED_API int corehost_resolve_component_dependencies( } } - // TODO: Need to redirect error writing (trace::error even with tracing disabled) - // to some local buffer and return the buffer to the caller as detailed error message. - // Like this the error is written to the stderr of the process which is pretty bad. - // It makes sense for startup code path as there's no other way to report it to the user. - // But with API call from managed code, the error should be invisible outside of exception. - // Tracing should still contain the error just like now. - // IMPORTANT: g_init is static/global and thus potentially accessed from multiple threads // We must only use it as read-only here (unlike the run scenarios which own it). // For example the frameworks in g_init.fx_definitions can't be used "as-is" by the resolver @@ -580,3 +573,30 @@ SHARED_API int corehost_resolve_component_dependencies( return 0; } + + +typedef void(*corehost_error_writer_fn)(const pal::char_t* message); + +// +// Sets a callback which is to be used to write errors to. +// +// Parameters: +// error_writer +// A callback function which will be invoked every time an error is to be reported. +// Or nullptr to unregister previously registered callback and return to the default behavior. +// Return value: +// The previously registered callback (which is now unregistered), or nullptr if no previous callback +// was registered +// +// The error writer is registered per-thread, so the registration is thread-local. On each thread +// only one callback can be registered. Subsequent registrations overwrite the previous ones. +// +// By default no callback is registered in which case the errors are written to stderr. +// +// Each call to the error writer is sort of like writing a single line (the EOL character is omitted). +// Multiple calls to the error writer may occure for one failure. +// +SHARED_API corehost_error_writer_fn corehost_set_error_writer(corehost_error_writer_fn error_writer) +{ + return trace::set_error_writer(error_writer); +} diff --git a/src/installer/corehost/common/pal.h b/src/installer/corehost/common/pal.h index dc97b83..67bcf5c 100644 --- a/src/installer/corehost/common/pal.h +++ b/src/installer/corehost/common/pal.h @@ -127,6 +127,7 @@ namespace pal inline void file_vprintf(FILE* f, const char_t* format, va_list vl) { ::vfwprintf(f, format, vl); ::fputwc(_X('\n'), f); } inline void err_vprintf(const char_t* format, va_list vl) { ::vfwprintf(stderr, format, vl); ::fputwc(_X('\n'), stderr); } inline void out_vprintf(const char_t* format, va_list vl) { ::vfwprintf(stdout, format, vl); ::fputwc(_X('\n'), stdout); } + inline int str_vprintf(char_t* buffer, size_t count, const char_t* format, va_list vl) { return ::_vsnwprintf(buffer, count, format, vl); } bool pal_utf8string(const pal::string_t& str, std::vector* out); bool utf8_palstring(const std::string& str, pal::string_t* out); @@ -173,6 +174,8 @@ namespace pal inline void file_vprintf(FILE* f, const char_t* format, va_list vl) { ::vfprintf(f, format, vl); ::fputc('\n', f); } inline void err_vprintf(const char_t* format, va_list vl) { ::vfprintf(stderr, format, vl); ::fputc('\n', stderr); } inline void out_vprintf(const char_t* format, va_list vl) { ::vfprintf(stdout, format, vl); ::fputc('\n', stdout); } + inline int str_vprintf(char_t* str, size_t size, const char_t* format, va_list vl) { return ::vsnprintf(str, size, format, vl); } + inline bool pal_utf8string(const pal::string_t& str, std::vector* out) { out->assign(str.begin(), str.end()); out->push_back('\0'); return true; } inline bool utf8_palstring(const std::string& str, pal::string_t* out) { out->assign(str); return true; } inline bool pal_clrstring(const pal::string_t& str, std::vector* out) { return pal_utf8string(str, out); } diff --git a/src/installer/corehost/common/trace.cpp b/src/installer/corehost/common/trace.cpp index 909565a..48ba919 100644 --- a/src/installer/corehost/common/trace.cpp +++ b/src/installer/corehost/common/trace.cpp @@ -14,6 +14,7 @@ static int g_trace_verbosity = 0; static FILE * g_trace_file = stderr; static std::mutex g_trace_mutex; +thread_local static trace::error_writer_fn g_error_writer = nullptr; // // Turn on tracing for the corehost based on "COREHOST_TRACE" & "COREHOST_TRACEFILE" env. @@ -122,7 +123,21 @@ void trace::error(const pal::char_t* format, ...) // Always print errors va_list args; va_start(args, format); - pal::err_vprintf(format, args); + + if (g_error_writer == nullptr) + { + pal::err_vprintf(format, args); + } + else + { + va_list dup_args; + va_copy(dup_args, args); + int count = pal::str_vprintf(NULL, 0, format, args) + 1; + std::vector buffer(count); + pal::str_vprintf(&buffer[0], count, format, dup_args); + g_error_writer(buffer.data()); + } + if (g_trace_verbosity && (g_trace_file != stderr)) { pal::file_vprintf(g_trace_file, format, args); @@ -166,3 +181,17 @@ void trace::flush() pal::err_flush(); pal::out_flush(); } + +trace::error_writer_fn trace::set_error_writer(trace::error_writer_fn error_writer) +{ + // No need for locking since g_error_writer is thread local. + error_writer_fn previous_writer = g_error_writer; + g_error_writer = error_writer; + return previous_writer; +} + +trace::error_writer_fn trace::get_error_writer() +{ + // No need for locking since g_error_writer is thread local. + return g_error_writer; +} \ No newline at end of file diff --git a/src/installer/corehost/common/trace.h b/src/installer/corehost/common/trace.h index d30e928..cbc2693 100644 --- a/src/installer/corehost/common/trace.h +++ b/src/installer/corehost/common/trace.h @@ -18,6 +18,18 @@ namespace trace void println(const pal::char_t* format, ...); void println(); void flush(); + + typedef void (*error_writer_fn)(const pal::char_t* message); + + // Sets a callback which is called whenever error is to be written + // The setting is per-thread (thread local). If no error writer is set for a given thread + // the error is written to stderr. + // The callback is set for the current thread which calls this function. + // The function returns the previously registered writer for the current thread (or null) + error_writer_fn set_error_writer(error_writer_fn error_writer); + + // Returns the currently set callback for error writing + error_writer_fn get_error_writer(); }; #endif // TRACE_H diff --git a/src/installer/test/Assets/TestProjects/HostApiInvokerApp/HostFXR.cs b/src/installer/test/Assets/TestProjects/HostApiInvokerApp/HostFXR.cs index 99d1a7d..dec4424 100644 --- a/src/installer/test/Assets/TestProjects/HostApiInvokerApp/HostFXR.cs +++ b/src/installer/test/Assets/TestProjects/HostApiInvokerApp/HostFXR.cs @@ -53,7 +53,16 @@ namespace HostApiInvokerApp string exe_dir, hostfxr_get_available_sdks_result_fn result); + internal const uint InvalidArgFailure = 0x80008081; internal const uint HostApiBufferTooSmall = 0x80008098; + + [UnmanagedFunctionPointer(CallingConvention.Cdecl, CharSet = Utils.OSCharSet)] + internal delegate void hostfxr_error_writer_fn( + string message); + + [DllImport(nameof(hostfxr), CharSet = Utils.OSCharSet, ExactSpelling = true, CallingConvention = CallingConvention.Cdecl)] + internal static extern IntPtr hostfxr_set_error_writer( + hostfxr_error_writer_fn error_writer); } /// @@ -78,15 +87,28 @@ namespace HostApiInvokerApp int required_buffer_size = 0; uint rc = 0; - for (int i = 0; i < 2; i++) + StringBuilder errorBuilder = new StringBuilder(); + + hostfxr.hostfxr_set_error_writer((message) => + { + errorBuilder.AppendLine(message); + }); + try { - rc = hostfxr.hostfxr_get_native_search_directories(argv.Length, argv, buffer, buffer.Capacity + 1, ref required_buffer_size); - if (rc != hostfxr.HostApiBufferTooSmall) + for (int i = 0; i < 2; i++) { - break; - } + rc = hostfxr.hostfxr_get_native_search_directories(argv.Length, argv, buffer, buffer.Capacity + 1, ref required_buffer_size); + if (rc != hostfxr.HostApiBufferTooSmall) + { + break; + } - buffer = new StringBuilder(required_buffer_size); + buffer = new StringBuilder(required_buffer_size); + } + } + finally + { + hostfxr.hostfxr_set_error_writer(null); } if (rc == 0) @@ -98,6 +120,54 @@ namespace HostApiInvokerApp { Console.WriteLine($"hostfxr_get_native_search_directories:Fail[{rc}]"); } + + if (errorBuilder.Length > 0) + { + Console.WriteLine($"hostfxr reported errors:{Environment.NewLine}{errorBuilder.ToString()}"); + } + } + + /// + /// Test invoking the native hostfxr api hostfxr_get_native_search_directories with invalid buffer + /// + static void Test_hostfxr_get_native_search_directories_invalid_buffer(string[] args) + { + StringBuilder errorBuilder = new StringBuilder(); + + hostfxr.hostfxr_set_error_writer((message) => + { + errorBuilder.AppendLine(message); + }); + + try + { + int required_buffer_size = 0; + Console.WriteLine("null buffer with non-zero size."); + uint rc = hostfxr.hostfxr_get_native_search_directories(0, null, null, 1, ref required_buffer_size); + Console.WriteLine($"hostfxr_get_native_search_directories error code: {rc}"); + if (rc != hostfxr.InvalidArgFailure) + { + throw new ApplicationException("hostfxr API should have returned InvalidArgFailure error code."); + } + + Console.WriteLine("negative buffer size."); + StringBuilder buffer = new StringBuilder(100); + rc = hostfxr.hostfxr_get_native_search_directories(0, null, buffer, -1, ref required_buffer_size); + Console.WriteLine($"hostfxr_get_native_search_directories error code: {rc}"); + if (rc != hostfxr.InvalidArgFailure) + { + throw new ApplicationException("hostfxr API should have returned InvalidArgFailure error code."); + } + } + finally + { + hostfxr.hostfxr_set_error_writer(null); + } + + if (errorBuilder.Length > 0) + { + Console.WriteLine($"hostfxr reported errors:{Environment.NewLine}{errorBuilder.ToString()}"); + } } /// @@ -161,6 +231,32 @@ namespace HostApiInvokerApp } } + static void Test_hostfxr_set_error_writer(string[] args) + { + hostfxr.hostfxr_error_writer_fn writer1 = (message) => { Console.WriteLine(nameof(writer1)); }; + IntPtr writer1Ptr = Marshal.GetFunctionPointerForDelegate(writer1); + + if (hostfxr.hostfxr_set_error_writer(writer1) != IntPtr.Zero) + { + throw new ApplicationException("Error writer should be null by default."); + } + + hostfxr.hostfxr_error_writer_fn writer2 = (message) => { Console.WriteLine(nameof(writer2)); }; + IntPtr writer2Ptr = Marshal.GetFunctionPointerForDelegate(writer2); + IntPtr previousWriterPtr = hostfxr.hostfxr_set_error_writer(writer2); + + if (previousWriterPtr != writer1Ptr) + { + throw new ApplicationException("First: The previous writer returned is not the one expected."); + } + + previousWriterPtr = hostfxr.hostfxr_set_error_writer(null); + if (previousWriterPtr != writer2Ptr) + { + throw new ApplicationException("Second: The previous writer returned is not the one expected."); + } + } + public static bool RunTest(string apiToTest, string[] args) { switch (apiToTest) @@ -174,6 +270,12 @@ namespace HostApiInvokerApp case nameof(hostfxr.hostfxr_get_available_sdks): Test_hostfxr_get_available_sdks(args); break; + case nameof(Test_hostfxr_set_error_writer): + Test_hostfxr_set_error_writer(args); + break; + case nameof(Test_hostfxr_get_native_search_directories_invalid_buffer): + Test_hostfxr_get_native_search_directories_invalid_buffer(args); + break; default: return false; } diff --git a/src/installer/test/Assets/TestProjects/HostApiInvokerApp/HostPolicy.cs b/src/installer/test/Assets/TestProjects/HostApiInvokerApp/HostPolicy.cs index 187ddfd..b58d365 100644 --- a/src/installer/test/Assets/TestProjects/HostApiInvokerApp/HostPolicy.cs +++ b/src/installer/test/Assets/TestProjects/HostApiInvokerApp/HostPolicy.cs @@ -1,8 +1,10 @@ using System; using System.Collections.Generic; +using System.IO; using System.Linq; using System.Runtime.InteropServices; using System.Text; +using System.Threading; namespace HostApiInvokerApp { @@ -20,45 +22,141 @@ namespace HostApiInvokerApp internal static extern int corehost_resolve_component_dependencies( string component_main_assembly_path, corehost_resolve_component_dependencies_result_fn result); + + [UnmanagedFunctionPointer(CallingConvention.Cdecl, CharSet = Utils.OSCharSet)] + internal delegate void corehost_error_writer_fn( + string message); + + [DllImport(nameof(hostpolicy), CharSet = Utils.OSCharSet, ExactSpelling = true, CallingConvention = CallingConvention.Cdecl)] + internal static extern IntPtr corehost_set_error_writer( + corehost_error_writer_fn error_writer); } - static void Test_corehost_resolve_component_dependencies(string[] args) + static void Test_corehost_resolve_component_dependencies_internal(string prefix, string assemblyPath) { - if (args.Length != 2) + StringBuilder errorBuilder = new StringBuilder(); + + hostpolicy.corehost_set_error_writer((message) => { - throw new ArgumentException("Invalid number of arguments passed"); - } + errorBuilder.AppendLine(message); + }); string assemblies = null; string nativeSearchPaths = null; string resourceSearcPaths = null; - int rc = hostpolicy.corehost_resolve_component_dependencies( - args[1], - (assembly_paths, native_search_paths, resource_search_paths) => - { - assemblies = assembly_paths; - nativeSearchPaths = native_search_paths; - resourceSearcPaths = resource_search_paths; - }); + int rc; + try + { + rc = hostpolicy.corehost_resolve_component_dependencies( + assemblyPath, + (assembly_paths, native_search_paths, resource_search_paths) => + { + assemblies = assembly_paths; + nativeSearchPaths = native_search_paths; + resourceSearcPaths = resource_search_paths; + }); + } + finally + { + hostpolicy.corehost_set_error_writer(null); + } if (assemblies != null) { // Sort the assemblies since in the native code we store it in a hash table // which gives random order. The native code always adds the separator at the end // so mimic that behavior as well. - assemblies = string.Join(System.IO.Path.PathSeparator, assemblies.Split(System.IO.Path.PathSeparator, StringSplitOptions.RemoveEmptyEntries).OrderBy(a => a)) + System.IO.Path.PathSeparator; + assemblies = string.Join(Path.PathSeparator, assemblies.Split(Path.PathSeparator, StringSplitOptions.RemoveEmptyEntries).OrderBy(a => a)) + Path.PathSeparator; } if (rc == 0) { - Console.WriteLine("corehost_resolve_component_dependencies:Success"); - Console.WriteLine($"corehost_resolve_component_dependencies assemblies:[{assemblies}]"); - Console.WriteLine($"corehost_resolve_component_dependencies native_search_paths:[{nativeSearchPaths}]"); - Console.WriteLine($"corehost_resolve_component_dependencies resource_search_paths:[{resourceSearcPaths}]"); + Console.WriteLine($"{prefix}corehost_resolve_component_dependencies:Success"); + Console.WriteLine($"{prefix}corehost_resolve_component_dependencies assemblies:[{assemblies}]"); + Console.WriteLine($"{prefix}corehost_resolve_component_dependencies native_search_paths:[{nativeSearchPaths}]"); + Console.WriteLine($"{prefix}corehost_resolve_component_dependencies resource_search_paths:[{resourceSearcPaths}]"); } else { - Console.WriteLine($"corehost_resolve_component_dependencies:Fail[0x{rc.ToString("X8")}]"); + Console.WriteLine($"{prefix}corehost_resolve_component_dependencies:Fail[0x{rc.ToString("X8")}]"); + } + + if (errorBuilder.Length > 0) + { + IEnumerable errorLines = errorBuilder.ToString().Split(new string[] { Environment.NewLine }, StringSplitOptions.None) + .Select(line => prefix + line); + Console.WriteLine($"{prefix}corehost reported errors:{Environment.NewLine}{string.Join(Environment.NewLine, errorLines)}"); + } + } + + static void Test_corehost_resolve_component_dependencies(string[] args) + { + if (args.Length != 2) + { + throw new ArgumentException("Invalid number of arguments passed"); + } + + Test_corehost_resolve_component_dependencies_internal("", args[1]); + } + + static void Test_corehost_resolve_component_dependencies_multithreaded(string[] args) + { + if (args.Length != 3) + { + throw new ArgumentException("Invalid number of arguments passed"); + } + + Func createThread = (string assemblyPath) => + { + return new Thread(() => + { + Test_corehost_resolve_component_dependencies_internal( + Path.GetFileNameWithoutExtension(assemblyPath) + ": ", + assemblyPath + ); + }); + }; + + Thread t1 = createThread(args[1]); + Thread t2 = createThread(args[2]); + + t1.Start(); + t2.Start(); + + if (!t1.Join(TimeSpan.FromSeconds(30))) + { + throw new ApplicationException("Thread 1 didn't finish in time."); + } + + if (!t2.Join(TimeSpan.FromSeconds(30))) + { + throw new ApplicationException("Thread 1 didn't finish in time."); + } + } + + static void Test_corehost_set_error_writer(string[] args) + { + hostpolicy.corehost_error_writer_fn writer1 = (message) => { Console.WriteLine(nameof(writer1)); }; + IntPtr writer1Ptr = Marshal.GetFunctionPointerForDelegate(writer1); + + if (hostpolicy.corehost_set_error_writer(writer1) != IntPtr.Zero) + { + throw new ApplicationException("Error writer should be null by default."); + } + + hostpolicy.corehost_error_writer_fn writer2 = (message) => { Console.WriteLine(nameof(writer2)); }; + IntPtr writer2Ptr = Marshal.GetFunctionPointerForDelegate(writer2); + IntPtr previousWriterPtr = hostpolicy.corehost_set_error_writer(writer2); + + if (previousWriterPtr != writer1Ptr) + { + throw new ApplicationException("First: The previous writer returned is not the one expected."); + } + + previousWriterPtr = hostpolicy.corehost_set_error_writer(null); + if (previousWriterPtr != writer2Ptr) + { + throw new ApplicationException("Second: The previous writer returned is not the one expected."); } } @@ -69,6 +167,12 @@ namespace HostApiInvokerApp case nameof(hostpolicy.corehost_resolve_component_dependencies): Test_corehost_resolve_component_dependencies(args); break; + case nameof(hostpolicy.corehost_resolve_component_dependencies) + "_multithreaded": + Test_corehost_resolve_component_dependencies_multithreaded(args); + break; + case nameof(Test_corehost_set_error_writer): + Test_corehost_set_error_writer(args); + break; default: return false; } diff --git a/src/installer/test/HostActivationTests/GivenThatICareAboutComponentDependencyResolution.cs b/src/installer/test/HostActivationTests/GivenThatICareAboutComponentDependencyResolution.cs index 67c9b9c..64ce90c 100644 --- a/src/installer/test/HostActivationTests/GivenThatICareAboutComponentDependencyResolution.cs +++ b/src/installer/test/HostActivationTests/GivenThatICareAboutComponentDependencyResolution.cs @@ -22,6 +22,7 @@ namespace Microsoft.DotNet.CoreSetup.Test.HostActivation.NativeHostApis } private const string corehost_resolve_component_dependencies = "corehost_resolve_component_dependencies"; + private const string corehost_resolve_component_dependencies_multithreaded = "corehost_resolve_component_dependencies_multithreaded"; [Fact] public void InvalidMainComponentAssemblyPathFails() @@ -39,7 +40,8 @@ namespace Microsoft.DotNet.CoreSetup.Test.HostActivation.NativeHostApis .StdErrAfter("corehost_resolve_component_dependencies = {") .Should().Pass() .And.HaveStdOutContaining("corehost_resolve_component_dependencies:Fail[0x80008092]") - .And.HaveStdErrContaining("Failed to locate managed application"); + .And.HaveStdOutContaining("corehost reported errors:") + .And.HaveStdOutContaining("Failed to locate managed application"); } [Fact] @@ -273,12 +275,15 @@ namespace Microsoft.DotNet.CoreSetup.Test.HostActivation.NativeHostApis .StdErrAfter("corehost_resolve_component_dependencies = {") .Should().Pass() .And.HaveStdOutContaining("corehost_resolve_component_dependencies:Fail[0x8000808C]") - .And.HaveStdErrContaining("An assembly specified in the application dependencies manifest (ComponentWithDependencies.deps.json) has already been found but with a different file extension") - .And.HaveStdErrContaining("package: 'ComponentDependency_Dupe', version: '1.0.0'") - .And.HaveStdErrContaining("path: 'ComponentDependency.notdll'") - .And.HaveStdErrContaining($"previously found assembly: '{Path.Combine(componentFixture.TestProject.OutputDirectory, "ComponentDependency.dll")}'"); + .And.HaveStdOutContaining("corehost reported errors:") + .And.HaveStdOutContaining("An assembly specified in the application dependencies manifest (ComponentWithDependencies.deps.json) has already been found but with a different file extension") + .And.HaveStdOutContaining("package: 'ComponentDependency_Dupe', version: '1.0.0'") + .And.HaveStdOutContaining("path: 'ComponentDependency.notdll'") + .And.HaveStdOutContaining($"previously found assembly: '{Path.Combine(componentFixture.TestProject.OutputDirectory, "ComponentDependency.dll")}'"); } + // This test also validates that corehost_set_error_writer custom writer + // correctly captures errors from hostpolicy. [Fact] public void ComponentWithCorruptedDepsJsonShouldFail() { @@ -301,8 +306,9 @@ namespace Microsoft.DotNet.CoreSetup.Test.HostActivation.NativeHostApis .StdErrAfter("corehost_resolve_component_dependencies = {") .Should().Pass() .And.HaveStdOutContaining("corehost_resolve_component_dependencies:Fail[0x8000808B]") - .And.HaveStdErrContaining($"A JSON parsing exception occurred in [{componentFixture.TestProject.DepsJson}]: * Line 1, Column 2 Syntax error: Malformed token") - .And.HaveStdErrContaining($"Error initializing the dependency resolver: An error occurred while parsing: {componentFixture.TestProject.DepsJson}"); + .And.HaveStdOutContaining("corehost reported errors:") + .And.HaveStdOutContaining($"A JSON parsing exception occurred in [{componentFixture.TestProject.DepsJson}]: * Line 1, Column 2 Syntax error: Malformed token") + .And.HaveStdOutContaining($"Error initializing the dependency resolver: An error occurred while parsing: {componentFixture.TestProject.DepsJson}"); } [Fact] @@ -378,6 +384,66 @@ namespace Microsoft.DotNet.CoreSetup.Test.HostActivation.NativeHostApis .And.HaveStdOutContaining($"corehost_resolve_component_dependencies assemblies:[{componentFixture.TestProject.AppDll}{Path.PathSeparator}]"); } + [Fact] + public void MultiThreadedComponentDependencyResolutionWhichSucceeeds() + { + var fixture = sharedTestState.PreviouslyPublishedAndRestoredPortableApiTestProjectFixture.Copy(); + var componentWithNoDependenciesFixture = sharedTestState.PreviouslyPublishedAndRestoredComponentWithNoDependenciesFixture.Copy(); + var componentWithResourcesFixture = sharedTestState.PreviouslyPublishedAndRestoredComponentWithResourcesFixture.Copy(); + + string componentWithNoDependenciesPrefix = Path.GetFileNameWithoutExtension(componentWithNoDependenciesFixture.TestProject.AppDll); + string componentWithResourcesPrefix = Path.GetFileNameWithoutExtension(componentWithResourcesFixture.TestProject.AppDll); + + string[] args = + { + corehost_resolve_component_dependencies_multithreaded, + componentWithNoDependenciesFixture.TestProject.AppDll, + componentWithResourcesFixture.TestProject.AppDll + }; + fixture.BuiltDotnet.Exec(fixture.TestProject.AppDll, args) + .CaptureStdOut().CaptureStdErr().EnvironmentVariable("COREHOST_TRACE", "1") + .Execute() + .Should().Pass() + .And.HaveStdOutContaining($"{componentWithNoDependenciesPrefix}: corehost_resolve_component_dependencies:Success") + .And.HaveStdOutContaining($"{componentWithNoDependenciesPrefix}: corehost_resolve_component_dependencies assemblies:[{componentWithNoDependenciesFixture.TestProject.AppDll}{Path.PathSeparator}]") + .And.HaveStdOutContaining($"{componentWithResourcesPrefix}: corehost_resolve_component_dependencies:Success") + .And.HaveStdOutContaining($"{componentWithResourcesPrefix}: corehost_resolve_component_dependencies resource_search_paths:[" + + $"{ExpectedProbingPaths(componentWithResourcesFixture.TestProject.OutputDirectory)}]"); + } + + [Fact] + public void MultiThreadedComponentDependencyResolutionWhichFailures() + { + var fixture = sharedTestState.PreviouslyPublishedAndRestoredPortableApiTestProjectFixture.Copy(); + var componentWithNoDependenciesFixture = sharedTestState.PreviouslyPublishedAndRestoredComponentWithNoDependenciesFixture.Copy(); + var componentWithResourcesFixture = sharedTestState.PreviouslyPublishedAndRestoredComponentWithResourcesFixture.Copy(); + + string componentWithNoDependenciesPrefix = Path.GetFileNameWithoutExtension(componentWithNoDependenciesFixture.TestProject.AppDll); + string componentWithResourcesPrefix = Path.GetFileNameWithoutExtension(componentWithResourcesFixture.TestProject.AppDll); + + // Corrupt the .deps.json by appending } to it (malformed json) + File.WriteAllText( + componentWithNoDependenciesFixture.TestProject.DepsJson, + File.ReadAllLines(componentWithNoDependenciesFixture.TestProject.DepsJson) + "}"); + + string[] args = + { + corehost_resolve_component_dependencies_multithreaded, + componentWithNoDependenciesFixture.TestProject.AppDll, + componentWithResourcesFixture.TestProject.AppDll + "_invalid" + }; + fixture.BuiltDotnet.Exec(fixture.TestProject.AppDll, args) + .CaptureStdOut().CaptureStdErr().EnvironmentVariable("COREHOST_TRACE", "1") + .Execute() + .Should().Pass() + .And.HaveStdOutContaining($"{componentWithNoDependenciesPrefix}: corehost_resolve_component_dependencies:Fail[0x8000808B]") + .And.HaveStdOutContaining($"{componentWithNoDependenciesPrefix}: corehost reported errors:") + .And.HaveStdOutContaining($"{componentWithNoDependenciesPrefix}: A JSON parsing exception occurred in [{componentWithNoDependenciesFixture.TestProject.DepsJson}]: * Line 1, Column 2 Syntax error: Malformed token") + .And.HaveStdOutContaining($"{componentWithNoDependenciesPrefix}: Error initializing the dependency resolver: An error occurred while parsing: {componentWithNoDependenciesFixture.TestProject.DepsJson}") + .And.HaveStdOutContaining($"{componentWithResourcesPrefix}: corehost_resolve_component_dependencies:Fail[0x80008092]") + .And.HaveStdOutContaining($"{componentWithResourcesPrefix}: corehost reported errors:") + .And.HaveStdOutContaining($"{componentWithResourcesPrefix}: Failed to locate managed application"); + } public class SharedTestState : IDisposable { diff --git a/src/installer/test/HostActivationTests/GivenThatICareAboutNativeHostApis.cs b/src/installer/test/HostActivationTests/GivenThatICareAboutNativeHostApis.cs index 30cbfe7..d108df5 100644 --- a/src/installer/test/HostActivationTests/GivenThatICareAboutNativeHostApis.cs +++ b/src/installer/test/HostActivationTests/GivenThatICareAboutNativeHostApis.cs @@ -45,6 +45,57 @@ namespace Microsoft.DotNet.CoreSetup.Test.HostActivation.NativeHostApis .HaveStdOutContaining("hostfxr_get_native_search_directories buffer:[" + dotnet.GreatestVersionSharedFxPath); } + // This test also validates that hostfxr_set_error_writer captures errors + // from hostfxr itself. + [Fact] + public void Hostfxr_get_native_search_directories_invalid_buffer() + { + var fixture = sharedTestState.PreviouslyPublishedAndRestoredPortableApiTestProjectFixture.Copy(); + + fixture.BuiltDotnet.Exec(fixture.TestProject.AppDll, "Test_hostfxr_get_native_search_directories_invalid_buffer") + .CaptureStdOut() + .CaptureStdErr() + .Execute() + .Should() + .Pass() + .And.HaveStdOutContaining($"hostfxr reported errors:") + .And.HaveStdOutContaining("hostfxr_get_native_search_directories received an invalid argument."); + } + + // This test also validates that hostfxr_set_error_writer propagates the custom writer + // to the hostpolicy.dll for the duration of those calls. + [Fact] + public void Muxer_hostfxr_get_native_search_directories_with_invalid_deps() + { + var fixture = sharedTestState.PreviouslyPublishedAndRestoredPortableApiTestProjectFixture.Copy(); + var appFixture = sharedTestState.PreviouslyPublishedAndRestoredPortableAppProjectFixture.Copy(); + var dotnet = fixture.BuiltDotnet; + + // Corrupt the .deps.json by appending } to it (malformed json) + File.WriteAllText( + appFixture.TestProject.DepsJson, + File.ReadAllLines(appFixture.TestProject.DepsJson) + "}"); + + var dotnetLocation = Path.Combine(dotnet.BinPath, $"dotnet{fixture.ExeExtension}"); + string[] args = + { + "hostfxr_get_native_search_directories", + dotnetLocation, + appFixture.TestProject.AppDll + }; + + dotnet.Exec(fixture.TestProject.AppDll, args) + .CaptureStdOut() + .CaptureStdErr() + .Execute() + .Should() + .Pass() + .And.HaveStdOutContaining("hostfxr_get_native_search_directories:Fail[2147516555]") // StatusCode::ResolverInitFailure + .And.HaveStdOutContaining("hostfxr reported errors:") + .And.HaveStdOutContaining($"A JSON parsing exception occurred in [{appFixture.TestProject.DepsJson}]: * Line 1, Column 2 Syntax error: Malformed token") + .And.HaveStdOutContaining($"Error initializing the dependency resolver: An error occurred while parsing: {appFixture.TestProject.DepsJson}"); + } + [Fact] public void Breadcrumb_thread_finishes_when_app_closes_normally() { @@ -265,6 +316,32 @@ namespace Microsoft.DotNet.CoreSetup.Test.HostActivation.NativeHostApis .HaveStdOutContaining($"hostfxr_resolve_sdk2 data:[{expectedData}]"); } + [Fact] + public void Hostfxr_corehost_set_error_writer_test() + { + var fixture = sharedTestState.PreviouslyPublishedAndRestoredPortableApiTestProjectFixture.Copy(); + + fixture.BuiltDotnet.Exec(fixture.TestProject.AppDll, "Test_hostfxr_set_error_writer") + .CaptureStdOut() + .CaptureStdErr() + .Execute() + .Should() + .Pass(); + } + + [Fact] + public void Hostpolicy_corehost_set_error_writer_test() + { + var fixture = sharedTestState.PreviouslyPublishedAndRestoredPortableApiTestProjectFixture.Copy(); + + fixture.BuiltDotnet.Exec(fixture.TestProject.AppDll, "Test_corehost_set_error_writer") + .CaptureStdOut() + .CaptureStdErr() + .Execute() + .Should() + .Pass(); + } + public class SharedTestState : IDisposable { public TestProjectFixture PreviouslyPublishedAndRestoredPortableApiTestProjectFixture { get; set; } -- 2.7.4