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

NetStandard2.0 and NetCoreApp3.0 supports added #119

Merged
merged 11 commits into from
Oct 4, 2019

Conversation

akselarzuman
Copy link
Contributor

@akselarzuman akselarzuman commented Sep 28, 2019

ref #118 #115 #111

Dotnet core 3.0 projects are being crashed while getting and setting _className in ExceptionSerializerFactory. _className value is always null but gets an exception in netcoreapp3.0 projects.

Moreover, getUninitializedObjectDelegate does not work with netcoreapp3.0 as well. Luckily FormatterServices is supported in netstandard2.0. In order to use it, I added netstandard2.0 support.

Summary

  • NetStandard2.0 support added in order to support Dotnet Core 3.0
  • NetCoreApp3.0 support added
  • Test project modified to run netcoreapp3.0 tests

@Horusiath Horusiath self-requested a review September 28, 2019 12:18
var className = stream.ReadString(session);
var message = stream.ReadString(session);
var remoteStackTraceString = stream.ReadString(session);
var stackTraceString = stream.ReadString(session);
var innerException = stream.ReadObject(session);

_className.SetValue(exception,className);
#if NETSTANDARD20
if (_className != null)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess this is an equivalent of _className?.SetValue(exception, className).


exceptionSerializer.Initialize((stream, session) =>
{
var exception = createInstance(type);
var exception = exceptionObject;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These two are not equivalent. createInstance(type) will return a new object on every deserializer call, while exceptionObject is an exception itself, so that the same instance will be captured and passed over and over. While this may not be obvious from simple unit tests, I'm not sure if this won't cause any troubles when called many times and from mutliple threads.

Do we have any way to prove that this change won't fail the running system?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To be honest I did not test that scenario but you have a point. Therefore, I reverted it back to Func. Please see my updated pull request.

@Horusiath Horusiath self-requested a review September 30, 2019 03:01
Copy link
Contributor

@Horusiath Horusiath left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks ok for me. The change to keep an eye on is a .NET standard upgrade v1.6 → v2.0. Waiting for @Aaronontheweb

@Danthar
Copy link
Member

Danthar commented Oct 3, 2019

PR validation fails because our build tooling still uses the dotnet core 2.2 runtime. You can update your PR probably by just changing which dotnetcore runtime we download in the tooling.
Checkout build.ps1 and build.sh for the proper variables. You might need to change some of the download commands as well.

For an example check this out: https://github.com/petabridge/Petabridge.App.Web

@akselarzuman
Copy link
Contributor Author

@Horusiath @Aaronontheweb @Danthar I have updated my PR. Validation should work well now. Can you please check?

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

Successfully merging this pull request may close these issues.

3 participants