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

[WIP] Moving towards .NET Standard/.NET Core compatibility #2134

Closed
wants to merge 1 commit into
from

Conversation

Projects
None yet
6 participants
@alexvaluyskiy
Member

alexvaluyskiy commented Jul 5, 2016

!Important! This PR is not about introducing NET CLI or xproj/project.json in the source code. It makes the source code easier to introduce .NET Platform compatibility in the future.

I used some ideas from this thread #992

Right now the sources match NetStandard 1.5, but it possible to achieve 1.3 in the future

@@ -295,7 +295,7 @@ private void CreateCoordinator()
foreach (var typeName in typeNames)
{
var rebalanceEnabled = string.Equals(typeName, "rebalancing", StringComparison.InvariantCultureIgnoreCase);
var rebalanceEnabled = string.Equals(typeName, "rebalancing", StringComparison.OrdinalIgnoreCase);

This comment has been minimized.

@alexvaluyskiy

alexvaluyskiy Jul 5, 2016

Member

MS recommends to replace InvariantCultureIgnoreCase to OrdinalIgnoreCase in many cases

@alexvaluyskiy

alexvaluyskiy Jul 5, 2016

Member

MS recommends to replace InvariantCultureIgnoreCase to OrdinalIgnoreCase in many cases

@@ -191,12 +191,16 @@ private static DedicatedThreadPoolSettings ConfigureSettings(Config config)
/// </summary>
internal sealed class ThreadPoolExecutorServiceFactory : ExecutorServiceConfigurator
{
#if UNSAFE_THREADING

This comment has been minimized.

@alexvaluyskiy

alexvaluyskiy Jul 5, 2016

Member

NetCore doesn't have AppDomains

@alexvaluyskiy

alexvaluyskiy Jul 5, 2016

Member

NetCore doesn't have AppDomains

var section = (AkkaConfigurationSection)System.Configuration.ConfigurationManager.GetSection("akka") ?? new AkkaConfigurationSection();
return section.AkkaConfig;
#else
return ConfigurationFactory.Empty;

This comment has been minimized.

@alexvaluyskiy

alexvaluyskiy Jul 5, 2016

Member

For compatibility reasons ConfigurationFactory.Load just returns empty config on NetCore

@alexvaluyskiy

alexvaluyskiy Jul 5, 2016

Member

For compatibility reasons ConfigurationFactory.Load just returns empty config on NetCore

ThreadType.Foreground : ThreadType.Background;
}
#if UNSAFE_THREADING
internal static ApartmentState GetApartmentState(Config cfg)

This comment has been minimized.

@alexvaluyskiy

alexvaluyskiy Jul 5, 2016

Member

NetCore versions doesn't have AppartmentState

@alexvaluyskiy

alexvaluyskiy Jul 5, 2016

Member

NetCore versions doesn't have AppartmentState

This comment has been minimized.

@alexvaluyskiy

alexvaluyskiy Jul 7, 2016

Member

@JeffCyr could we live without AppartmentState on NetCore?

@alexvaluyskiy

alexvaluyskiy Jul 7, 2016

Member

@JeffCyr could we live without AppartmentState on NetCore?

@@ -304,15 +325,21 @@ public PoolWorker(DedicatedThreadPool pool, int workerId)
_pool = pool;
_threadExit = new TaskCompletionSource<object>();
#if UNSAFE_THREADING
var thread = new Thread(RunThread, pool.Settings.ThreadMaxStackSize);

This comment has been minimized.

@alexvaluyskiy

alexvaluyskiy Jul 5, 2016

Member

NetCore version doesn't have a parameter for ThreadMaxStackSize

@alexvaluyskiy

alexvaluyskiy Jul 5, 2016

Member

NetCore version doesn't have a parameter for ThreadMaxStackSize

This comment has been minimized.

@alexvaluyskiy

alexvaluyskiy Jul 7, 2016

Member

@JeffCyr, what do you think about it?

@alexvaluyskiy

alexvaluyskiy Jul 7, 2016

Member

@JeffCyr, what do you think about it?

This comment has been minimized.

@JeffCyr

JeffCyr Jul 8, 2016

Contributor

I think it's not configurable from akka anyway, LGTM

@JeffCyr

JeffCyr Jul 8, 2016

Contributor

I think it's not configurable from akka anyway, LGTM

@@ -338,10 +338,12 @@ private void ProcessAllSystemMessages()
msg.Unlink();
DebugPrint("{0} processing system message {1} with {2}", Actor.Self, msg, string.Join(",", Actor.GetChildren()));
// we know here that SystemInvoke ensures that only "fatal" exceptions get rethrown
#if UNSAFE_THREADING

This comment has been minimized.

@alexvaluyskiy

alexvaluyskiy Jul 5, 2016

Member

Should I ignore these exceptions on NetCore?

@alexvaluyskiy

alexvaluyskiy Jul 5, 2016

Member

Should I ignore these exceptions on NetCore?

This comment has been minimized.

@JeffCyr

JeffCyr Jul 7, 2016

Contributor

As far as I know the ThreadInterruptedException and ThreadAbortException handlers here are useless.

Actor.SystemInvoke isn't blocking, so ThreadInterruptedException can't be thrown.

In the ThreadAbortException handler, Thread.ResetAbort() isn't called so the exception will be re-thrown automatically.

ThreadAbort resistance is very hard to get right and I don't think it is properly implemented here even if Thread.ResetAbort was called, I would advise to simply remove thoses exceptions handlers and never abort threads.

@JeffCyr

JeffCyr Jul 7, 2016

Contributor

As far as I know the ThreadInterruptedException and ThreadAbortException handlers here are useless.

Actor.SystemInvoke isn't blocking, so ThreadInterruptedException can't be thrown.

In the ThreadAbortException handler, Thread.ResetAbort() isn't called so the exception will be re-thrown automatically.

ThreadAbort resistance is very hard to get right and I don't think it is properly implemented here even if Thread.ResetAbort was called, I would advise to simply remove thoses exceptions handlers and never abort threads.

This comment has been minimized.

@alexvaluyskiy

alexvaluyskiy Jul 8, 2016

Member

@Aaronontheweb could I remove these two handlers?

@alexvaluyskiy

alexvaluyskiy Jul 8, 2016

Member

@Aaronontheweb could I remove these two handlers?

@@ -559,7 +559,7 @@ private string PullEscapeSequence()
case 'u':
string hex = "0x" + Take(4);
int j = Convert.ToInt32(hex, 16);
return ((char) j).ToString(CultureInfo.InvariantCulture);
return ((char) j).ToString();

This comment has been minimized.

@alexvaluyskiy

alexvaluyskiy Jul 5, 2016

Member

I'm not sure that I did the right conversion

@alexvaluyskiy

alexvaluyskiy Jul 5, 2016

Member

I'm not sure that I did the right conversion

This comment has been minimized.

@alexvaluyskiy

alexvaluyskiy Jul 8, 2016

Member

The code from ReferenceSource

[Pure]
public String ToString(IFormatProvider provider) { 
    Contract.Ensures(Contract.Result<String>() != null);
    return Char.ToString(m_value); 
} 

MS doesn't use this parameter

@alexvaluyskiy

alexvaluyskiy Jul 8, 2016

Member

The code from ReferenceSource

[Pure]
public String ToString(IFormatProvider provider) { 
    Contract.Ensures(Contract.Result<String>() != null);
    return Char.ToString(m_value); 
} 

MS doesn't use this parameter

Show outdated Hide outdated src/core/Akka/Actor/Address.cs
@@ -22,7 +22,7 @@ namespace Akka.Actor
/// for example a remote transport would want to associate additional
/// information with an address, then this must be done externally.
/// </summary>
public sealed class Address : ICloneable, IEquatable<Address>, ISurrogated

This comment has been minimized.

@alexvaluyskiy

alexvaluyskiy Jul 5, 2016

Member

Should I remove ICloneable interface or use compilation directives?

@alexvaluyskiy

alexvaluyskiy Jul 5, 2016

Member

Should I remove ICloneable interface or use compilation directives?

@alexvaluyskiy alexvaluyskiy added this to the 1.5.0 milestone Jul 5, 2016

@alexvaluyskiy alexvaluyskiy changed the title from [WIP] Moving towards .NET Standard compatibility to [WIP] Moving towards .NET Standard/.NET Core compatibility Jul 5, 2016

@alexvaluyskiy alexvaluyskiy modified the milestones: 1.2.0, 1.5.0 Jul 6, 2016

@jpsingleton jpsingleton referenced this pull request Jul 7, 2016

Closed

Add akka.net #4

@alexvaluyskiy

This comment has been minimized.

Show comment
Hide comment
@alexvaluyskiy

alexvaluyskiy Jul 7, 2016

Member

@akkadotnet/developers So, the main question. What should we do with BinarySerialization in exceptions, and with SerializableAttribute? We could just remove it, as many projects did. Or we can add compilation directives, as I did here

Akka.Net doesn't use BinarySerialization. So, why we are using these attributes?

Member

alexvaluyskiy commented Jul 7, 2016

@akkadotnet/developers So, the main question. What should we do with BinarySerialization in exceptions, and with SerializableAttribute? We could just remove it, as many projects did. Or we can add compilation directives, as I did here

Akka.Net doesn't use BinarySerialization. So, why we are using these attributes?

@Aaronontheweb

This comment has been minimized.

Show comment
Hide comment
@Aaronontheweb

Aaronontheweb Jul 7, 2016

Member

paging @JeffCyr - questions about unsafe threading on .NET Core :p

Member

Aaronontheweb commented Jul 7, 2016

paging @JeffCyr - questions about unsafe threading on .NET Core :p

@@ -165,7 +165,7 @@ public static GraphInterpreter Current
get
{
if (CurrentInterpreter.Value[0] == null)
throw new ApplicationException("Something went terribly wrong!");
throw new InvalidOperationException("Something went terribly wrong!");

This comment has been minimized.

@alexvaluyskiy

alexvaluyskiy Jul 7, 2016

Member

NetCore doesn't have ApplicationException, changed to InvalidOperationException

@alexvaluyskiy

alexvaluyskiy Jul 7, 2016

Member

NetCore doesn't have ApplicationException, changed to InvalidOperationException

@@ -65,15 +64,17 @@ public void The_default_configuration_file_contain_all_configuration_properties(
settings.SchedulerClass.ShouldBe(typeof(DedicatedThreadScheduler).FullName);
}
#if CONFIGURATION
[Fact]
public void Deserializes_hocon_configuration_from_net_config_file()

This comment has been minimized.

@alexvaluyskiy

alexvaluyskiy Jul 7, 2016

Member

Skipped this test for NetCore

@alexvaluyskiy

alexvaluyskiy Jul 7, 2016

Member

Skipped this test for NetCore

@@ -23,7 +23,7 @@ public override int GetHashCode()
public override string ToString()
{
var res = fastJSON.JSON.Instance.ToJSON(this);
var res = Newtonsoft.Json.JsonConvert.SerializeObject(this);

This comment has been minimized.

@alexvaluyskiy

alexvaluyskiy Jul 7, 2016

Member

Replaced fastJson to NetwonsoftJson

@alexvaluyskiy

alexvaluyskiy Jul 7, 2016

Member

Replaced fastJson to NetwonsoftJson

@@ -69,7 +72,23 @@ internal class DedicatedThreadPoolSettings
if (numThreads <= 0)
throw new ArgumentOutOfRangeException("numThreads", string.Format("numThreads must be at least 1. Was {0}", numThreads));
}
#else
public DedicatedThreadPoolSettings(int numThreads, string name = null, TimeSpan? deadlockTimeout = null)

This comment has been minimized.

@alexvaluyskiy

alexvaluyskiy Jul 8, 2016

Member

Added a different constructor for NetCore version, without ApartmentState

@alexvaluyskiy

alexvaluyskiy Jul 8, 2016

Member

Added a different constructor for NetCore version, without ApartmentState

@adamhathcock

This comment has been minimized.

Show comment
Hide comment
@adamhathcock

adamhathcock Jul 9, 2016

Contributor

Say it ain't so!

Contributor

adamhathcock commented Jul 9, 2016

Say it ain't so!

@alexvaluyskiy

This comment has been minimized.

Show comment
Hide comment
@alexvaluyskiy

alexvaluyskiy Jul 9, 2016

Member

We should create a feature branch here instead of this PR

Member

alexvaluyskiy commented Jul 9, 2016

We should create a feature branch here instead of this PR

@Aaronontheweb

This comment has been minimized.

Show comment
Hide comment
@Aaronontheweb

Aaronontheweb Jul 9, 2016

Member

@alexvaluyskiy at the next contributor's meeting, let's sort that out for how we want to do this incrementally.

Member

Aaronontheweb commented Jul 9, 2016

@alexvaluyskiy at the next contributor's meeting, let's sort that out for how we want to do this incrementally.

@alexvaluyskiy

This comment has been minimized.

Show comment
Hide comment
Member

alexvaluyskiy commented Jul 9, 2016

@Aaronontheweb Ok.
Right now I'm working on it in my local branch
https://github.com/alexvaluyskiy/akka.net/commits/netcorecodefixes

@alexvaluyskiy alexvaluyskiy removed this from the 1.2.0 milestone Jul 27, 2016

@wmeints

This comment has been minimized.

Show comment
Hide comment
@wmeints

wmeints Aug 6, 2016

Contributor

@sdanishali No it does not, it only supports full .NET framework. This is mentioned in the comments of the post :-) Sorry dude, it's going to take some time before Akka.NET supports the .NET standard way of doing things. Until then use net45 or higher instead.

Contributor

wmeints commented Aug 6, 2016

@sdanishali No it does not, it only supports full .NET framework. This is mentioned in the comments of the post :-) Sorry dude, it's going to take some time before Akka.NET supports the .NET standard way of doing things. Until then use net45 or higher instead.

@alexvaluyskiy alexvaluyskiy deleted the alexvaluyskiy:netcore branch May 7, 2017

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