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 document on Mono embedding API deprecation #47715

Closed
wants to merge 1 commit into from

Conversation

CoffeeFlux
Copy link
Contributor

Draft for now, since there are some yet-unresolved questions at the bottom.

This is a fairly simple proposal, but should hopefully serve as a nice start for discussion. I think our stance on deprecation in mono/mono was overly conservative, and I'd like to come to some kind of agreement on how to handle things in .NET 5+ before .NET 6 ships.

I'm not sure how this would affect consoles since I don't know what the release schedule there is like. @lateralusX can hopefully chime in on that.


How does this work with LTS releases? Should we try and deprecate on LTS and remove on non-LTS when possible, or does it not matter?

Should we include in the function declaration what release a given function was added or deprecated in? This would probably take the form of replacing `MONO_API` with `SINCE` and `DEPRECATED` calls, but increases complexity and effort for potentially limited value.
Copy link
Member

@lambdageek lambdageek Feb 1, 2021

Choose a reason for hiding this comment

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

I don't know about replacing MONO_API by "deprecated" and "since" but I think it would be good to have that information available in the headers (as opposed to having to hunt it down from git). (I don't think we need to annotate every existing symbol, but as we deprecate/add symbols, it would be nice to note the release)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I personally think we should either enforce it via the defines or it may not be applied consistently. You'd use MONO_API for all the existing functions, but anything new uses SINCE or DEPRECATED instead, and that forces people to be conscious of the change. I don't feel that strongly about it, however.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Upon reflection, I think having a mix in public headers would probably not be great. I think it would be better to replace existing MONO_API calls with SINCE(net5) and then use the new defines going forward. MONO_API would be fine for exported functions not present in a public header.

Copy link
Member

@lambdageek lambdageek Feb 2, 2021

Choose a reason for hiding this comment

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

I guess I'm unclear why we need to change MONO_API.
Can't we have something like this:

MONO_RT_EXTERNAL_ONLY
MONO_API
MONO_API_DEPRECATED(net6, "safe to remove in all callers")
void mono_domain_set (MonoDomain *ignored);

MONO_RT_EXTERNAL_ONLY
MONO_API
MONO_API_SINCE(net6)
void mono_shiny_new_thing (void);

Which are defined as

#define MONO_API_DEPRECATED(dep_release, replacement_msg)` \
  __attribute__((deprecated(replacement_msg ", the function will be removed in the release following " #dep_release))

#define MONO_API_SINCE(since) /* empty */


API stability is more complicated. There are two distinct cases here: .NET 6 and the behavior moving forward.

With the .NET 6 release specifically, we have an opportunity to do some initial API cleanup work because relatively few desktop users have migrated over to .NET 5 Mono. There is some low-hanging fruit to potentially clean up immediately, before we have significant migration: irrelevant functionality like `mono_domain_set` or `refonly` parameters, legacy compatibility hacks like `MonoLookupPInvokeStatus`, and subtly-incorrect behavior like fixed-size GC handles.
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure that "most embedding users are on net4x, and therefore we can set the bar for migration higher" is a compelling argument. To me it sounds like net6 would have to be solely focused deprecation and offering an optional migration path, but not outright removing symbols.

Although in some cases the deprecated symbols might have semantic changes (e.g. that refonly parameter when it's not-FALSE can't possibly have the same behavior as in net4x).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm. Maybe I'm just trying to justify a desire to immediately get rid of stuff that I think should have been removed years ago like MonoLookupPInvokeStatus, and that's not the right balance here. I dunno though, I think that with .NET Core there's precedent for "rip out the stuff that makes no sense under the new design" and/or should been gone years ago, and I'm not actually sure we're making life that much easier for embedders if we continue to allow calls to things like mono_domain_set...

Copy link
Member

Choose a reason for hiding this comment

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

I'm not actually sure we're making life that much easier for embedders if we continue to allow calls to things like mono_domain_set

That's not really a typical example, though. There's a clear easy good replacement for mono_domain_set - a no-op.

Some function that returns a value that makes sense in some circumstances but not others may be less obvious. Take mono_lookup_pinvoke_call, for example, if I have a ton of code to handle the "exception class, exception arg" out-arg case, is it obvious how to rewrite it in terms of something that uses MonoError (presumably our replacement API)?

(digression: I assume you don't literally mean MonoLookupPInvokeStatus - it isn't a public API - but mono_lookup_pinvoke_call which is the ugly API with the exception name out parameter that MonoLookupPInvokeStatus is meant to paper over)

Also what is the upgrade path from net4x to net6 like now for an embedder if we remove functions: you have to work around everything we removed before you can even try your code to see if it still works with System.Private.CoreLib / the new BCL / new managed API surface etc.


# Points for further discussion (to address before merging)

For deprecated APIs that include useless arguments or are irrelevant for netcore, should we silently ignore them/do nothing or throw a runtime error? What would be more useful for embedders, given that the deprecations allow them to detect potentially problematic functions at compile time?
Copy link
Member

Choose a reason for hiding this comment

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

Hard to generalize without a specific example, but what about a gradual scheme (this will require deprecation across multiple releases):

  1. when the function is marked deprecated we initially try to preserve behavior (ie, refonly == FALSE works as before, refonly == TRUE makes the mono_assembly_load function return null and set MonoImageOpenStatus to non-zero)
  2. (optional) next release, same semantics, but also print a warning.
  3. next release (or when we think it's appropriate), assert refonly == FALSE
  4. next release (or when we think it's appropriate), finally, remove the symbol

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This strikes me as too complicated, and I think it's unlikely that we'll carefully manage a bunch of steps like that in practice. I feel pretty strongly that we should be able to just go deprecation->removal.

Copy link
Member

Choose a reason for hiding this comment

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

I feel pretty strongly that we should be able to just go deprecation->removal.

I think in practice that will raise the bar on deprecation to the point where we won't deprecate anything.

This strikes me as too complicated, and I think it's unlikely that we'll carefully manage a bunch of steps like that in practice.

But I also agree with this. ¯\(ツ)

docs/design/mono/embedding-api-deprecation.md Show resolved Hide resolved

Deprecation is the preferred path for functions likely to see heavy use with embedders in .NET 6 and the _only_ option past that point. .NET 6 is the only release where removal may occur without the attribute first being present in a major release, though this should be avoided with functions likely to see heavy use in existing embedding workloads.

# Points for further discussion (to address before merging)
Copy link
Member

@jkotas jkotas Feb 2, 2021

Choose a reason for hiding this comment

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

The .NET Framework and CoreCLR approach for these scenarios has been to tell people to wrap whatever managed functionality they need as PInvoke or reverse PInvoke. To complement that, we have been exposing functionality that is required to make this work as managed API (e.g. function pointers or SuppressGCTransitionAttribute added in .NET 5 fall into this category).

Would it be reasonable to gradually switch Mono over to this plan as well? We can make that easier by e.g. creating samples, etc.


Any function appearing in a major release marked deprecated may, and likely will go away in the following major release. This means that if `mono_assembly_load` is deprecated for .NET 6, it can be removed at any time leading up to .NET 7 or beyond, including during the previews.

Deprecation is the preferred path for functions likely to see heavy use with embedders in .NET 6 and the _only_ option past that point. .NET 6 is the only release where removal may occur without the attribute first being present in a major release, though this should be avoided with functions likely to see heavy use in existing embedding workloads.
Copy link
Member

Choose a reason for hiding this comment

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

It would be useful to do some customer development with embedders to validate that these assumptions are correct.

@migueldeicaza
Copy link
Contributor

.NET might choose to not support that API, that does not mean that Mono needs to deprecate that API.

@CoffeeFlux
Copy link
Contributor Author

There is an increasingly significant cost to maintaining some of the APIs, as shown by the recent NativeLibrary compatibility hacks, and many APIs are not even relevant or functional in .NET 5.

I do not consider the strategy of never deprecating to be sustainable. If we want to actually maintain the embedding API and have it be useful for customers in .NET 5, it has to be able to slowly evolve. Having it become increasingly broken or an increasing time sink at the expense of improving the product in other ways does not seem like the right balance to strike, and given our limited resources those are what I see as the realistic scenarios (and how it's been playing out so far).

None of this will affect mono/mono, which will almost certainly maintain its current API surface forever. .NET 5 gives us a chance to strike a different balance.

Base automatically changed from master to main March 1, 2021 09:07
@ghost
Copy link

ghost commented Mar 31, 2021

Draft Pull Request was automatically closed for inactivity. It can be manually reopened in the next 30 days if the work resumes.

@ghost ghost closed this Mar 31, 2021
@ghost ghost locked as resolved and limited conversation to collaborators Apr 30, 2021
@karelz karelz added this to the 6.0.0 milestone May 20, 2021
This pull request was closed.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

5 participants