Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use ObjectHandle instead of calling ToString in NLogBeginScopeParser #237

Closed
snakefoot opened this issue Aug 20, 2018 · 2 comments
Closed
Labels

Comments

@snakefoot
Copy link
Contributor

snakefoot commented Aug 20, 2018

Consider adding support for ObjectHandle in NLog (With Unwrap), instead of the ToString-hack used in #232:

https://github.com/dotnet/corefx/blob/master/src/System.Diagnostics.DiagnosticSource/src/System/Diagnostics/Activity.Current.net45.cs

If NLog supports ObjectHandle with Unwrap, then this silly code can be removed:

if (state?.GetType().IsSerializable ?? true)
return NestedDiagnosticsLogicalContext.Push(state);
else
return NestedDiagnosticsLogicalContext.Push(state.ToString()); // Support HostingLogScope, ActionLogScope, FormattedLogValues and others

Maybe something like this:

            INestedContext CreateNestedContext(INestedContext parent, T value)
            {
#if NET4_6 || NETSTANDARD
                return new NestedContext<T>(parent, value);
#else
                if (value is IConvertible)
                    return new NestedContext<T>(parent, value);
                else
                    return new NestedContext<System.Runtime.Remoting.ObjectHandle>(parent, new System.Runtime.Remoting.ObjectHandle((object)value));
#endif
            }

            object INestedContext.Value
            {
                get
                {
#if NET4_6 || NETSTANDARD
                    return Value;
#else
                    if (Value is System.Runtime.Remoting.IObjectHandle objectHandle)
                        return objectHandle.Unwrap();
                    else
                        return Value;
#endif
                }
            }

This will also make NLog more user-friendly for others using MDLC and NDLC

@304NotModified
Copy link
Member

I don't know much of IObjectHandle , but it sounds good.

Do I need to read into this (IObjectHandle )?

@snakefoot
Copy link
Contributor Author

snakefoot commented Aug 21, 2018

@304NotModified Do I need to read into this (IObjectHandle )?

No idea. Can just see that it is the official hack for handling object-sharing across AppDomains. Instead of activating the remoting-serialization for accross-process-communication (Not a remoting or callcontext guru).

Created NLog/NLog#2857 and used the medium-trust test to show that it explodes when given complex objects (But ObjectHandle fixes it).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants