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

Store Activity wrapped into ObjectHandle in Logical CallContext #20963

Merged
merged 1 commit into from
Jun 14, 2017

Conversation

lmolkova
Copy link

@lmolkova lmolkova commented Jun 13, 2017

This change addresses issue when an application has Activity stored in the logical call context and makes a call to another AppDomain that do not have DiagnosticSource.dll loaded.
In this case, an application may crash failing to find DiagnosticSource.dll. This issue potentially affects Azure CloudService users on NET .45 that use some features (role environments change events) of WindowsAzure.ServiceRuntime and ApplicationInsights

So this change stores Activity wrapped into ObjectHandle in the logical call context.
First of all, it allows using AppDomains that do not load DiagnosticSource. It also allows to remove Serializable attribute (see below why it's a good thing) as ObjectHandle implements MarshalByRefObject.

Supporting [Serializable] Activity across AppDomains creates several issues:

  1. Copy of the Activity is created when Activity is serialized and it may lead to collisions in Activity.Id generation.
  2. If users will start to serialize Activity while they are on 4.5 and later upgrade to 4.6+, their code will stop working, as @stephentoub noticed, since it's not serializable on .net46
  3. On other .NET versions, Activity uses AsyncLocal to flow Current, that does not flow across AppDomains. Only Activity.Id and Baggage are supposed to propagate. Flowing it between AppDomains is not a requirement, on the contrary, it is inconsistent behavior.

So, this change does not allow different AppDomains reuse Activity.
This is also important to allow wrapping Activity into ObjectHandle: Activity does not inherit from MarshalByRefObject and could not be used remotely by reference anyway.
Changing Activity to inherit from the MarshalByRefObject creates some complications and does not bring any value (see p3 above).

Activity was never released as stable and chances that someone already uses it in cross AppDomain calls on .NET 4.5 are inconceivable.

These changes were tested manually on .NET 4.5. Automation is in progress in partner team infrastructure (ApplicationInsights).

@vancem @SergeyKanzhelev @stephentoub please review

Fixes #21027

@vancem vancem merged commit eaa04bd into dotnet:master Jun 14, 2017
@vancem
Copy link
Contributor

vancem commented Jun 14, 2017

LGTM

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

Successfully merging this pull request may close these issues.

5 participants