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

Add FormatterServices.GetUninitializedObject #17144

Closed
KrzysztofCwalina opened this issue Apr 27, 2016 · 30 comments
Closed

Add FormatterServices.GetUninitializedObject #17144

KrzysztofCwalina opened this issue Apr 27, 2016 · 30 comments
Assignees
Labels
api-approved API was approved in API review, it can be implemented area-System.Runtime
Milestone

Comments

@KrzysztofCwalina
Copy link
Member

The current plan is to not support binary serialization natively in .NET Core, but we would like to enable the community and partners (e.g. distributed computing framework providers), and possibly our own out-of-band libraries (BinaryFromatter in a compatibility pack published to nuget) to implement something similar to what binary serialization is doing on top of .NET Core.

So far, we identified a couple of enabler features that we do need to add to the core to make such out-of-band serializers possible:

  1. FormatterServices.GetUninitializedObject
  2. Exception.set_StackTrace

This issue/request is to add these two asap. They are absolutely blocking and required. We might need more as we work with partners and community on enabling their scenarios.

See the following issues for more context: #17073, #16563,

@KrzysztofCwalina
Copy link
Member Author

@weshaggard, as we discussed, you asked me to open this issue and you said you can find somebody who could implement it. Thanks!

@eiriktsarpalis
Copy link
Member

I think I've mentioned this elsewhere, but if it were up to me I'd opt to bring back ISerializable (and having System.Exception re-implement it) instead of making the stack trace mutable by just about everybody.

While I do believe that ISerializable as a pattern is dated and carries many caveats, it is ideally suited in cases where inheritance is involved. And since .NET exceptions necessarily rely on inheritance, why not stick to that approach?

@KrzysztofCwalina
Copy link
Member Author

@eiriktsarpalis, I love more discussion input on this, but please use #16563 for general discussion around serialization. This issue is a specific API addition request. Thanks!

@ah-
Copy link
Contributor

ah- commented Apr 28, 2016

+1. Will this be in a netstandard so libraries can use it?

@KrzysztofCwalina
Copy link
Member Author

GetUninitializedObject will be in the current net standard. For set_StackTrace, it's harder. It will be in netstandard, but maybe not in the current version as the API does not exist in .NET Framework.

@terrajobst
Copy link
Member

For set_StackTrace I assume we'd throw for cases where a stack trace has already been created? I think we shouldn't allow replacing stack traces as it could result in us no longer trusting stack traces for tricky bugs.

@KrzysztofCwalina
Copy link
Member Author

@terrajobst, sounds like a good place to start.

@weshaggard weshaggard assigned danmoseley and unassigned weshaggard Apr 30, 2016
@SGuyGe
Copy link
Contributor

SGuyGe commented May 11, 2016

We need FormatterServices.GetUninitializedObject for sure. For Exception.set_StackTrace, I don't think that alone would fulfill our need to serialize Exception types. There're more members that we need to set, e.g. Exception.Message, Exception.InnerException. And there're even more members in the derived Exceptions, e.g. ArgumentException.ParamName. If we want to go this route, then we'll need to provide setters for all properties of Exception and its descendants.

danmoseley referenced this issue in dotnet/corefx May 16, 2016
danmoseley referenced this issue in dotnet/corefx May 16, 2016
Add FormatterServices.GetUninitializedObject, partial for #8133
@danmoseley
Copy link
Member

Breaking out StackTrace into a separate issue dotnet/corefx#9294. Updated title.

@danmoseley danmoseley changed the title Add FormatterServices.GetUninitializedObject and Exception.set_StackTrace Add FormatterServices.GetUninitializedObject Jun 9, 2016
@niemyjski
Copy link

Any idea on where I can get formatter services? I need this today!

@ah-
Copy link
Contributor

ah- commented Jun 24, 2016

If you really need it you can just get it via reflection, it's just not exposed.

@weshaggard
Copy link
Member

This is already exposed in dev/api branch.

@stephentoub @danmosemsft do we still have some clean-up work in the implementation?

@danmoseley
Copy link
Member

@weshaggard That's right (it's only accessible via reflection). Do you recall why you asked me to revert it? (dotnet/corefx#8741)

@stephentoub
Copy link
Member

We access it via reflection here:
https://github.com/dotnet/corefx/blob/dev/api/src/System.Runtime.Serialization.Formatters/src/System/Runtime/Serialization/FormatterServices.cs#L70-L75
This is the FormatterServices in corefx (the one we actually expose) using reflection to access the FormatterServices in coreclr. The right fix is to change it to use the RuntimeHelpers.GetUnintializedObject method that @jkotas exposed here:
https://github.com/dotnet/coreclr/blob/master/src/mscorlib/src/System/Runtime/CompilerServices/RuntimeHelpers.cs#L31
If it hasn't already been exposed from corert as well, that will need to be done, too.

@danmoseley danmoseley assigned tarekgh and unassigned danmoseley Aug 30, 2016
@danmoseley
Copy link
Member

@tarekgh Please expose GetUninitializedObject in corefx. As Stephen notes this may need exposing in CoreRT first.

@stephentoub
Copy link
Member

stephentoub commented Aug 30, 2016

Please expose GetUninitializedObject in corefx

I don't know that this is desirable (I'm assuming you mean RuntimeHelpers.GetUninitializedObject... FormatterServices.GetUninitializedObject is already exposed in corefx... it's just using reflection in its GetUninitializedObject implementation to access the "real" implementation in coreclr). @jkotas can correct me, but I believe the idea is that it's more of an implementation detail, a public method exposed from coreclr/corert that can be used by corefx but that isn't actually exposed in public corefx contracts.

@jkotas
Copy link
Member

jkotas commented Aug 30, 2016

I think it is a good idea to expose RuntimeHelpers.GetUninitializedObject in the public contract. We wanted to do it for .NET Core 1.0, but it was not possible because of the contract versioning deadlock at the time.

We want to have enough capabilities in the public lower level contracts to support custom serialization engines, without pulling in full binary serialization (ie FormatterServices) into their dependency closure.

@jkotas
Copy link
Member

jkotas commented Aug 30, 2016

may need exposing in CoreRT first

The raw functionality exists in CoreRT as well (RuntimeAugments.NewObject), it is just not exported under the right name and with the right argument validation.

However, filling in the CoreRT side is generally trailing behind CoreCLR. I do not think exposing it in CoreRT has to block doing the right thing (tm) for CoreCLR.

@stephentoub
Copy link
Member

I think it is a good idea to expose RuntimeHelpers.GetUninitializedObject in the public contract.

Ok. It shouldn't be necessary to enable this FormatterServices reflection removal, but doing that first will make this easier, as if it's exposed from System.Runtime's public contract, then the Formatters library can use that rather than needing to be changed to build against the mscorlib/System.Private.Corelib façade.

@weshaggard
Copy link
Member

We can expose RuntimeHelpers.GetUninitializedObject only for .NET Core but we cannot put it in the netstandard reference assembly because it doesn't exist anywhere but .NET Core right now. However we could expose FormatterServices.GetUninitializedObject if we wanted as that does exist in other places. I like the idea of RuntimeHelpers version but I don't think it helps us much right now so for now I would just expose it on FormatterServices (which I suspect it already is) and then let the serialization library call through that instead of using reflection.

@jkotas
Copy link
Member

jkotas commented Aug 30, 2016

build against the mscorlib/System.Private.Corelib façade

I guess that this is the best option for now, given the constrains.

@weshaggard
Copy link
Member

tagged as netfx-port-consider as we need to expose RuntimeHelpers.GetUninitializedObject on desktop as well.

@tarekgh
Copy link
Member

tarekgh commented Nov 14, 2016

after talking offline with @weshaggard here is the plan and some info for FormatterServices.GetUninitializedObject

  • RuntimeHelpers.GetUninitializedObject is already exposed from system.Runtime in netcoreapp1.1
  • We'll not change the implementation of FormatterServices.GetUninitializedObject which is currently using the reflection when building the library as netstandard. there is already some discussion about building the libraries for specific platforms (netcoreapp, UWA...etc) and get rid of building the libraries as netstandard. anyway, if we continue building the library as netsatndard we want to avoid building it against the corelib.
  • we'll add a build configuration System.Runtime.Serialization.Formatters to build for netcoreapp1.1 and we'll change the implementation of FormatterServices.GetUninitializedObject to call RuntimeHelpers.GetUninitializedObject instead of using the reflection.

@stephentoub
Copy link
Member

anyway, if we continue building the library as netsatndard we want to avoid building it against the corelib

Can you help me to understand why?

@tarekgh
Copy link
Member

tarekgh commented Nov 14, 2016

@weshaggard could you please help answering @stephentoub question? Thx.

@weshaggard
Copy link
Member

Can you help me to understand why?

We are technically lying to ourselves if we are building a library targeting netstandard that has a direct dependency on S.P.CoreLib, as that by nature causes it to not be supported anywhere that doesn't have a S.P.CoreLib.

@stephentoub
Copy link
Member

stephentoub commented Nov 14, 2016

We are technically lying to ourselves if we are building a library targeting netstandard that has a direct dependency on S.P.CoreLib, as that by nature causes it to not be supported anywhere that doesn't have a S.P.CoreLib.

Maybe I'm misunderstanding something. Isn't System.Runtime.Serialization.Formatters part of .NET Core's implementation of netstandard, not implemented on top of netstandard?

@stephentoub
Copy link
Member

stephentoub commented Nov 14, 2016

Also, whether it has a direct dependency to S.P.Corelib or uses reflection to access what's effectively an internal type of Corelib (it's public, but it's not part of the netstandard surface area), isn't it a similar lie?

@weshaggard
Copy link
Member

Maybe I'm misunderstanding something. Isn't System.Runtime.Serialization.Formatters part of .NET Core's implementation of netstandard, not implemented on top of netstandard?

Yes it is part of netstandard2.0 but it is also building against netstandard (https://github.com/dotnet/corefx/blob/master/src/System.Runtime.Serialization.Formatters/src/System.Runtime.Serialization.Formatters.csproj#L9) which we should instead switch to netcoreapp in order to access the new RuntimeHelper API. As a bigger push we will end up disabling/deleting all the netstandard build configurations for things that are part of netstandard.

Also, whether it has a direct dependency to Sp.Corelib or uses reflection to access what's effectively an internal type of Corelib (it's public, but it's not part of the netstandard surface area), isn't it a similar lie?

It is a similar lie but by using reflection it is a light-up based dependency that can fail (or fallback) on platforms that might not have that API. Whereas if it had an assembly reference to S.P.CoreLib the entire assembly would fail to load on a platform that didn't have that assembly.

@tarekgh
Copy link
Member

tarekgh commented Nov 14, 2016

I have did the changes in System.Runtime.Serialization.Formatters adn now when compiling as netcoreapp1.1 we are using RuntimeHelpers.GetUninitializedObject and not using the reflection.

Also this issue is marked as netfx-port-consider so would be tracked by the tools to do the work for the full framework which is exposing RuntimeHelpers.GetUninitializedObject

by that I am closing the issue now as there is no more action needed.

@tarekgh tarekgh closed this as completed Nov 14, 2016
@msftgits msftgits transferred this issue from dotnet/corefx Jan 31, 2020
@msftgits msftgits added this to the 2.0.0 milestone Jan 31, 2020
@ghost ghost locked as resolved and limited conversation to collaborators Jan 1, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
api-approved API was approved in API review, it can be implemented area-System.Runtime
Projects
None yet
Development

No branches or pull requests