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

=str Make SubFlow and SubSource a final class. #619

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

He-Pin
Copy link
Member

@He-Pin He-Pin commented Sep 2, 2023

Motivation:

Those two class should be final.

Update:
I still think those classes should be marked final and was marked as final in Akka since 2.7.0 and no one complain. Who will extends those two classes outside of pekko ?

@mdedetrich
Copy link
Contributor

Motivation:

Those two class should be final.

What's the reasoning behind them needing to be final, there may be some rare usecase to subclass it

@He-Pin
Copy link
Member Author

He-Pin commented Sep 2, 2023

Motivation:

Those two class should be final.

What's the reasoning behind them needing to be final, there may be some rare usecase to subclass it

Forbidden user extension and final class sometimes faster too.

@pjfanning
Copy link
Contributor

I'm -1 on this

  • breaks bin compatibility
  • no proven perf gain
  • what's wrong with letting users extend classes?

@He-Pin
Copy link
Member Author

He-Pin commented Sep 2, 2023

MiMA check can be fixed.

Extends this class will not work as no extension point, they are not marked because of a out of sight, not because of it was right.

+1 for this;

  1. introduce this will not break any real thing, who will do that.
  2. I was there, who wanted to extends this classes.
  3. prevent future user error.

@jxnu-liguobin
Copy link
Member

+1
When I really inherit this class, I should want to override something, except for direct use of delegate, what can I do?

If we don't know if it can be inherited, we should close it. Although it is possible to lose flexibility, but this is potential flexibility, but also we avoid potential dangers.

Copy link
Contributor

@mdedetrich mdedetrich left a comment

Choose a reason for hiding this comment

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

I am going to block this PR for now because making public non final classes final is technically a breaking change since if any user happened to extend SubFlow/SubSource their code will no longer work if they were to bring in a version of Pekko with this change. Its only in Pekko 2.0.x that such a change can be merged, and we are not even at that stage yet. You can't just retroactively break code for users because you think that they shouldn't extend these classes, breaking users is breaking users and in doing so we are ignoring semver.

Also the merits of this PR is somewhat dubious wrt performance. The JVM performs CHA (class hierarchy analysis) at runtime which means that all classes are effectively final if the JVM detects that there are no subclasses of that class when the program is being run. In other words if this is a hotspot, the JVM will inline this to static methods at runtime if no one extends SubFlow/SubSource.

To me, legitimate reasons to use final is if by design contract you don't want users to extend the class (because they can break the design in some fundamental way). Scala's case class is a good example of this, i.e. they should always be final because otherwise people can override methods such as equals which would create lots of nasty surprises. The Scala 2 inliner is another reason why you may want to use final (depending on the context, the Scala 2 inliner may only work with final).

Now it could be that legitimately speaking a user should never extend SubFlow/SubSource however if this is the case as said before it would have needed to be done before Pekko's first 1.0.0 release (too late for that now) or in 2.0.x.

@mdedetrich
Copy link
Contributor

Unlike Akka we follow semver which means we can only change this in 2.0.x.

Also I doubt this has any actual impact as JVM will make it effectively final as long as no one subclasses it.

@He-Pin
Copy link
Member Author

He-Pin commented Jan 13, 2024

Ok, let's schedule it to 2.0.x, at least I think no one will be affected.

@He-Pin He-Pin added this to the 2.0.x milestone Jan 13, 2024
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.

None yet

4 participants