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

Add System.AppDomain apis #11275

Merged
merged 1 commit into from Sep 13, 2016

Conversation

Projects
None yet
@rahku
Copy link
Member

rahku commented Aug 30, 2016

currently build fails as System.Reflection.Assembly should be present in system.runtime.dll. So waiting for PR#11099

@rahku

This comment has been minimized.

Copy link
Member Author

rahku commented Aug 30, 2016

fixes issue #9327

private static AppDomain domain = new AppDomain();
private AppDomain() {}
public static AppDomain CurrentDomain => domain;
public Assembly[] GetAssemblies() => AppDomainAugments.GetAssemblies();

This comment has been minimized.

@jkotas

jkotas Aug 31, 2016

Member

This should rather be: AssemblyLoadContext.Default.GetAssemblies();

This comment has been minimized.

@jkotas

jkotas Aug 31, 2016

Member

Or AssemblyLoadContext.GetAllAssemblies(); - if we want this to return all assemblies ever loaded anywhere.

{
add
{
AppDomainAugments.UnhandledException += value;

This comment has been minimized.

@jkotas

jkotas Aug 31, 2016

Member

I would put the UnhandledException handler on EnvironmentAugments - I think it would fit better there.

{
public partial class AppDomain
{
private static AppDomain domain = new AppDomain();

This comment has been minimized.

@stephentoub

stephentoub Aug 31, 2016

Member

Nit: s_domain

And readonly

This comment has been minimized.

@jkotas

jkotas Aug 31, 2016

Member

Or if you would like to be cool, you can write it on single line as: public static AppDomain CurrentDomain { get; } = new AppDomain(); ;-)

public static AppDomain CurrentDomain => domain;
public Assembly[] GetAssemblies() => AppDomainAugments.GetAssemblies();
public string BaseDirectory => "";
public string RelativeSearchPath => "";

This comment has been minimized.

@stephentoub

stephentoub Aug 31, 2016

Member

Are empty strings the right thing to return here? What do folks normally use these values for?

This comment has been minimized.

@rahku

rahku Aug 31, 2016

Author Member

These actually do not make sense for coreclr. They were present in AppDomain for desktop wherein you have AppDomainSetup & AppDomainManager to set these values. So in the absense of AppDomainSetup & AppDomainManager for coreclr you cannot actually change values for BaseDirectory & RelativeSearchPath and hence it will always return empty string. We are exposing these properties (even though it returns epmpty string) for binary compat. But I also see a similar property in AppContext

public static string BaseDirectory { get { return default(string); } }
. So now it does not make sense to add this here as well. @danmosemsft what do you think?

This comment has been minimized.

@jkotas

jkotas Aug 31, 2016

Member

What these properties do in CoreCLR today is irrelevant. The code for them happens to be there as internal detail, not exposed as public surface. The implementation is not necessarily correct.

The behavior of these properties on other runtimes is what matters. On other runtimes, the BaseDirectory property returns the "application directory", not an empty string, even for simple console applications without AppDomainSetup and AppDomainManager. The "application directory" is already returned by AppContext.BaseDirectory, so it makes sense for the AppDomain implementation to forward to it.

public object ExceptionObject { get { throw null; } }
public bool IsTerminating { get { throw null; } }
}
public delegate void UnhandledExceptionEventHandler(object sender, System.UnhandledExceptionEventArgs e);

This comment has been minimized.

@stephentoub

stephentoub Aug 31, 2016

Member

Why is this in a separate contract from AppDomain?

This comment has been minimized.

@weshaggard

weshaggard Aug 31, 2016

Member

I agree this should be where it is used. The only reason we might put it somewhere else is if we need to share it, perhaps if we expose this on AppContext that might require putting this in a shared location.

@rahku rahku changed the title Add System.AppDomain apis [WIP] Add System.AppDomain apis Aug 31, 2016

@rahku rahku force-pushed the rahku:appdomain branch from e4aa8e1 to 0d4bb7f Aug 31, 2016

private AppDomain() {}
public static AppDomain CurrentDomain => domain;
public Assembly[] GetAssemblies() => AppDomainAugments.GetAssemblies();
public string BaseDirectory => "";

This comment has been minimized.

@jkotas

jkotas Aug 31, 2016

Member

Should this return AppContext.BaseDirectory instead?

This comment has been minimized.

@weshaggard
@@ -8,6 +8,16 @@

namespace System
{
public partial class AppDomain

This comment has been minimized.

@weshaggard

weshaggard Aug 31, 2016

Member

Why did you put this in this contract?

This comment has been minimized.

@danmosemsft

danmosemsft Aug 31, 2016

Member

@weSh where do you suggest? Krzysztof suggested a new contract (System.Runtime.Remoting) but I don't think we're going to create a new one.


In reply to: 76925026 [](ancestors = 76925026)

This comment has been minimized.

@weshaggard

weshaggard Aug 31, 2016

Member

At this point I was just asking the question to see why it was chosen, but I'm not sure were we should put it yet. There is a decent chance that everything in System.Runtime.Extensions gets folded into System.Runtime and when that happens I'm not sure I want AppDomain to be part of that. So we would have to keep it here or separate it at that point. I'm wondering if we shouldn't just have a System.AppDomain library that is independent.

This comment has been minimized.

@jkotas

jkotas Aug 31, 2016

Member

everything in System.Runtime.Extensions gets folded into System.Runtime

Would it make sense to keep System.Runtime.Extensions for things like AppDomain? Fold just what make sense - not everything.

This comment has been minimized.

@weshaggard

weshaggard Sep 1, 2016

Member

I'm good going with that as the plan until we find a reason it doesn't work (if we find one).

@danmosemsft

This comment has been minimized.

Copy link
Member

danmosemsft commented Sep 6, 2016

@rahku Note that Assembly et al is now available (#11386)

@rahku rahku force-pushed the rahku:appdomain branch from 0d4bb7f to 0273c36 Sep 7, 2016

@rahku

This comment has been minimized.

Copy link
Member Author

rahku commented Sep 7, 2016

Yeah I am currently looking if it will be possible to support unhandledexception event for reverse pinvoke case on linux. On windows we depend on OS api to support it which is absent on linux.

@@ -44,6 +45,19 @@ public static object GetData(string name)
return null;
}

public static event UnhandledExceptionEventHandler UnhandledException

This comment has been minimized.

@weshaggard

weshaggard Sep 8, 2016

Member

FYI this implementation is .NET Native specific as the CoreCLR AppContext is in System.Private.CoreLib.

This comment has been minimized.

@jkotas

jkotas Sep 8, 2016

Member

We will want to move the AppContext implementation to System.Private.CoreLib for CoreRT as well.

This comment has been minimized.

@weshaggard

weshaggard Sep 8, 2016

Member

I don't believe we will need to but it really depends on how we implement some of the stuff like UnhandledException. I'm good with moving AppContext out of S.P.CoreLib even on CoreCLR if that works, I've not looked at it in detail to see if there are any issues with that. If we do move it out then we can share more of the code in corefx and we would then need some "Augments" type in CoreLib for things like this.

This comment has been minimized.

@jkotas

jkotas Sep 8, 2016

Member

how we implement some of the stuff like UnhandledException

UnhandledException handling needs to live in S.P.CoreLib.

Most methods on AppContext today are tightly coupled with the runtime, host or toolchain. You cannot really move it out for S.P.CoreLib for CoreCLR unless you introduce Augments class in CoreLib that most of the methods will forward to. The fact that it is separate for PN is false sense of layering.

This comment has been minimized.

@weshaggard

weshaggard Sep 8, 2016

Member

OK we should just fold it into the System.Runtime contract and S.P.CoreLib implementation.

@rahku rahku force-pushed the rahku:appdomain branch from 10a3491 to ea07bef Sep 8, 2016

@rahku rahku changed the title [WIP] Add System.AppDomain apis Add System.AppDomain apis Sep 8, 2016

@rahku

This comment has been minimized.

Copy link
Member Author

rahku commented Sep 8, 2016

Can you please review the changes. I plan to add some tests which actually cause unhandled exception in coreclr repo as there isn't a way to test this in xunit. I will wait for merge till System.private.corelib changes gets updated.

public void CurrentDomain_Not_Null()
{
Assert.NotNull(AppDomain.CurrentDomain);
}

This comment has been minimized.

@danmosemsft

danmosemsft Sep 8, 2016

Member

} [](start = 8, length = 1)

nit. 2 line spacing

[Fact]
public void UnhandledException_NotCalled_When_Handled()
{
AppContext.UnhandledException += new UnhandledExceptionEventHandler(MyHandler);

This comment has been minimized.

@danmosemsft

danmosemsft Sep 8, 2016

Member

MyHandler [](start = 80, length = 9)

should this (and below) be NotExpectedToBeCalledHandler

@rahku rahku force-pushed the rahku:appdomain branch from ea07bef to 2bc58cc Sep 8, 2016

@rahku

This comment has been minimized.

Copy link
Member Author

rahku commented Sep 9, 2016

@dotnet-bot retest this please

public void RelativeSearchPath_Is_Null()
{
Assert.Null(AppDomain.RelativeSearchPath);
}

This comment has been minimized.

@jkotas

jkotas Sep 9, 2016

Member

Are the tests for UnhandledException coming too?

@jkotas

This comment has been minimized.

Copy link
Member

jkotas commented Sep 9, 2016

LGTM otherwise

@rahku rahku force-pushed the rahku:appdomain branch 2 times, most recently from 11937f7 to c910e05 Sep 12, 2016

@rahku

This comment has been minimized.

Copy link
Member Author

rahku commented Sep 12, 2016

@weshaggard PTAL. All checks are green now finally

@rahku

This comment has been minimized.

Copy link
Member Author

rahku commented Sep 12, 2016

@dotnet-bot test Innerloop OSX Debug Build and Test

@rahku rahku removed the * NO MERGE * label Sep 13, 2016

"dependencies": {
"Microsoft.TargetingPack.Private.CoreCLR": "1.1.0-beta-24512-03"
},
"imports": [
"dotnet5.6"

This comment has been minimized.

@weshaggard

weshaggard Sep 13, 2016

Member

This is the wrong mapping. It isn't actually needed so you can remove all the imports in this file.

@@ -100,6 +101,10 @@
<Project>{1e689c1b-690c-4799-bde9-6e7990585894}</Project>
<Name>System.Runtime.Extensions.CoreCLR</Name>
</ProjectReference>
<ProjectReference Include="..\..\System.Runtime\pkg\System.Runtime.pkgproj">
</ProjectReference>
<ProjectReference Include="..\..\System.AppContext\pkg\System.AppContext.pkgproj">

This comment has been minimized.

@weshaggard

weshaggard Sep 13, 2016

Member

Why is System.AppContext needed here?

This comment has been minimized.

@rahku

rahku Sep 13, 2016

Author Member

I get this error otherwise because now AppContext exists in both assemblies.
System\AppDomainTests.cs(29,65): error CS0433: The type 'AppContext' exists in both 'System.AppContext, Version=4.1.0.0, Culture=neutral, PublicKeyToken=b03f5f7f11d50a3a' and 'System.Runtime, Version=4.2.0.0, Culture=neutral, PublicKeyToken=b03f5f7f11d50a3a' [F:\corefxgit\corefx\src\System.Runtime.Extensions\tests\System.Runtime.Extensions.Tests.csproj]

@@ -100,6 +101,10 @@
<Project>{1e689c1b-690c-4799-bde9-6e7990585894}</Project>
<Name>System.Runtime.Extensions.CoreCLR</Name>
</ProjectReference>
<ProjectReference Include="..\..\System.Runtime\pkg\System.Runtime.pkgproj">

This comment has been minimized.

@weshaggard

weshaggard Sep 13, 2016

Member

We should have a tracking issue to changes this ProjectReferences to pkgprojs with package references instead after the necessary packages are published.

This comment has been minimized.

@rahku

rahku Sep 13, 2016

Author Member

created issue #11680

MembersMustExist : Member 'System.Type.IsSecuritySafeCritical.get()' does not exist in the implementation but it does exist in the contract.
MembersMustExist : Member 'System.Type.IsSecurityTransparent.get()' does not exist in the implementation but it does exist in the contract.
MembersMustExist : Member 'System.Reflection.Pointer..ctor()' does not exist in the implementation but it does exist in the contract.
MembersMustExist : Member 'System.AppContext.TargetFrameworkName.get()' does not exist in the implementation but it does exist in the contract.

This comment has been minimized.

@weshaggard

weshaggard Sep 13, 2016

Member

FYI @ericstj - These AppContext APIs can be in .NET Core but not .NET Standard yet. We will need to change these these ref projects to .NET Core.

This comment has been minimized.

@ericstj

ericstj Sep 13, 2016

Member

Ok, file an issue.

@rahku rahku force-pushed the rahku:appdomain branch from c910e05 to 6b977e7 Sep 13, 2016

@rahku

This comment has been minimized.

Copy link
Member Author

rahku commented Sep 13, 2016

@dotnet-bot test Innerloop OSX Release Build and Test

@rahku rahku force-pushed the rahku:appdomain branch from 6b977e7 to e1311a2 Sep 13, 2016

@rahku rahku merged commit 64df6e5 into dotnet:dev/api Sep 13, 2016

0 of 10 checks passed

Innerloop CentOS7.1 Debug Build and Test Triggered.
Details
Innerloop CentOS7.1 Release Build and Test Started.
Details
Innerloop Linux ARM Emulator Debug Cross Build Triggered.
Details
Innerloop Linux ARM Emulator Release Cross Build Triggered.
Details
Innerloop OSX Debug Build and Test Started.
Details
Innerloop OSX Release Build and Test Started.
Details
Innerloop Ubuntu14.04 Debug Build and Test Triggered.
Details
Innerloop Ubuntu14.04 Release Build and Test Triggered.
Details
Innerloop Windows_NT Debug Build and Test Triggered.
Details
Innerloop Windows_NT Release Build and Test Triggered.
Details

@rahku rahku deleted the rahku:appdomain branch Sep 24, 2016

@karelz karelz modified the milestone: 1.2.0 Dec 3, 2016

@gitfortee

This comment has been minimized.

Copy link

gitfortee commented Oct 10, 2017

@rahku @karelz how do i find which version of .net core has support for AppDomain.UnhandledException?

@danmosemsft

This comment has been minimized.

Copy link
Member

danmosemsft commented Oct 10, 2017

@gitfortee you need version 2.0

@karelz

This comment has been minimized.

Copy link
Member

karelz commented Oct 10, 2017

@gitfortee you can tell from commits, or from milestone on the PR, or from docs, or from https://apisof.net.

@gitfortee

This comment has been minimized.

Copy link

gitfortee commented Oct 11, 2017

@rahku is there an example usage of AppDomain.CurrentDomain.UnhandledException? there is a ExceptionObject property.. how to extract information out of it.?

@karelz

This comment has been minimized.

Copy link
Member

karelz commented Oct 11, 2017

@gitfortee

This comment has been minimized.

Copy link

gitfortee commented Oct 11, 2017

@karelz i will check this out.

@birbilis

This comment has been minimized.

Copy link

birbilis commented Oct 19, 2018

regarding the UnhandledException handler, according to that doc link:
"Starting with the .NET Framework 4, this event is not raised for exceptions that corrupt the state of the process, such as stack overflows or access violations, unless the event handler is security-critical and has the HandleProcessCorruptedStateExceptionsAttribute attribute."
...so not exactly a replacement for try/catch. Wonder if the behavior in .NET Core is this one or the older .NET one.

BTW, doc was quite long and from a quick look couldn't find what happens if code in the exception handler causes a new exception to be raised. Is there any case it can fall in spurious loop (aka should people always place their handler code inside try/catch?)

Another thing I don't get with the ExceptionObject is why it gives one an object that they cast to Exception. All examples I've seen cast to Exception, but if there is any case something else can be passed via the ExceptionObject it will fail (which led to the previous question on whether it will do spurious loop in that case)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment