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

8273101: Eliminate the usage of threadgroup sandboxing in the java.util.logging #5326

Closed
wants to merge 4 commits into from

Conversation

mrserb
Copy link
Member

@mrserb mrserb commented Sep 1, 2021

At the time Java supported applets and webstart, a special mechanism for launching various applications in one JVM was used to reduce memory usage and each application was isolated from each other.

This isolation was implemented via ThreadGroups where each application created its own thread group and all data for the application was stored in the context of its own group.

Some parts of the JDK used ThreadGroups directly, others used special classes like sun.awt.AppContext.

Such sandboxing became useless since the plugin and webstart are no longer supported and were removed in jdk11.

Note that this is just a first step for the overall cleanup, here is my plan:

  1. Cleanup the usage of AppContext in the “java.util.logging.LogManager" class in the java.base module.
  2. Cleanup the usage of TheadGroup in the JavaSound
  3. Cleanup the usage of TheadGroup in the JavaBeans
  4. Cleanup the usage of AppContext in the Swing
  5. Cleanup the usage of AppContext in the AWT
  6. Delete the AppContext class.

The current PR is for the first step. So this is the request to delete the usage of AppContext in the LogManager and related tests.

Tested by tier1/tier2/tier3 and jdk_desktop tests.


Progress

  • Change must not contain extraneous whitespace
  • Commit message must refer to an issue
  • Change must be properly reviewed

Issue

  • JDK-8273101: Eliminate the usage of threadgroup sandboxing in the java.util.logging

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.java.net/jdk pull/5326/head:pull/5326
$ git checkout pull/5326

Update a local copy of the PR:
$ git checkout pull/5326
$ git pull https://git.openjdk.java.net/jdk pull/5326/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 5326

View PR using the GUI difftool:
$ git pr show -t 5326

Using diff file

Download this PR as a diff file:
https://git.openjdk.java.net/jdk/pull/5326.diff

@bridgekeeper
Copy link

@bridgekeeper bridgekeeper bot commented Sep 1, 2021

👋 Welcome back serb! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

@openjdk
Copy link

@openjdk openjdk bot commented Sep 1, 2021

@mrserb The following labels will be automatically applied to this pull request:

  • awt
  • core-libs
  • security

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing lists. If you would like to change these labels, use the /label pull request command.

@openjdk openjdk bot added security core-libs awt labels Sep 1, 2021
@mrserb mrserb marked this pull request as ready for review Sep 2, 2021
@openjdk openjdk bot added the rfr label Sep 2, 2021
@mlbridge
Copy link

@mlbridge mlbridge bot commented Sep 2, 2021

Webrevs

@prrace
Copy link
Contributor

@prrace prrace commented Sep 3, 2021

This fix requires coordinated closed changes so needs an Oracle sponsor.
And actually is probably better handled entirely by an Oracle engineer because pushes need to be very co-ordinated.

@prrace
Copy link
Contributor

@prrace prrace commented Sep 3, 2021

/reviewers 2

@openjdk
Copy link

@openjdk openjdk bot commented Sep 3, 2021

@prrace
The number of required reviews for this PR is now set to 2 (with at least 1 of role reviewers).

@prrace
Copy link
Contributor

@prrace prrace commented Sep 3, 2021

I believe we should have a CSR for this. It removes a long standing capability in the platform that was used by
components such as plugin and webstart.

/csr

@openjdk openjdk bot added the csr label Sep 3, 2021
@openjdk
Copy link

@openjdk openjdk bot commented Sep 3, 2021

@prrace has indicated that a compatibility and specification (CSR) request is needed for this pull request.
@mrserb please create a CSR request for issue JDK-8273101. This pull request cannot be integrated until the CSR request is approved.

@prrace
Copy link
Contributor

@prrace prrace commented Sep 3, 2021

Hmm I was under the impression this was removing AppContext itself but it is just removing the backdoor needed by logging
Perhaps this isn't the change that requires the CSR but it then leaves an inconsistent state where desktop supports AppContext still but other modules don't ...

@prrace
Copy link
Contributor

@prrace prrace commented Sep 3, 2021

@aivanov-jdk will help make sure the closed changes are pushed at exactly the same time as this gets pushed.

@prrace
Copy link
Contributor

@prrace prrace commented Sep 3, 2021

/csr unneeded

@openjdk openjdk bot removed the csr label Sep 3, 2021
@openjdk
Copy link

@openjdk openjdk bot commented Sep 3, 2021

@prrace determined that a CSR request is not needed for this pull request.

@mrserb
Copy link
Member Author

@mrserb mrserb commented Sep 3, 2021

Perhaps this isn't the change that requires the CSR but it then leaves an inconsistent state where desktop supports AppContext still but other modules don't ...

Even java.desktop does not support it fully, since for a couple of years nobody care about appcontext when write a new code.
Note that I mentioned the "threadgroup sandboxing" in the subject, which is not necessary implemented via Appcontext. The JavaBeans and JavaSound use the thread group directly, I plan to remove them as well.

@dfuch
Copy link
Member

@dfuch dfuch commented Sep 3, 2021

Right. I am also a bit uncomfortable about the inconsistency. That said - if everybody agrees that this should be done I have no objection. The changes to the java.util.logging implementation correspond to my expectation if this were to be carried through.

@@ -1,77 +0,0 @@
/*
Copy link
Member

@dfuch dfuch Sep 3, 2021

Choose a reason for hiding this comment

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

Humm... There's no direct reference to AppContext here. Maybe this test should be preserved?

Copy link
Member Author

@mrserb mrserb Sep 3, 2021

Choose a reason for hiding this comment

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

It uses it indirectly, the next line under security manager trigger creation of the appcontext:
ImageIO.read(is); // triggers calls to system loggers & creation of main AppContext

but I can preserve/rename it for sure.

@AlanBateman
Copy link
Contributor

@AlanBateman AlanBateman commented Sep 6, 2021

No objection to removing this legacy/unused code but I think it would be useful to useful to have the JBS issue or the PR summary provide a bit more context. As I see it, this is just one piece of the overall cleanup and I assume there are more substantive changes to the java.desktop module coming, is that right?

@mrserb
Copy link
Member Author

@mrserb mrserb commented Sep 6, 2021

As I see it, this is just one piece of the overall cleanup and I assume there are more substantive changes to the java.desktop module coming, is that right?

Yes, you are right.

@mlchung
Copy link
Member

@mlchung mlchung commented Sep 9, 2021

Hmm I was under the impression this was removing AppContext itself but it is just removing the backdoor needed by logging
Perhaps this isn't the change that requires the CSR but it then leaves an inconsistent state where desktop supports AppContext still but other modules don't ...

What is the plan to remove AppContext completely from java.desktop? It's okay to have a separate PR to remove the logging dependency on AppContext but I'd prefer to see AppContext be removed about the same time with this PR.

@mrserb
Copy link
Member Author

@mrserb mrserb commented Sep 9, 2021

I'll remove its used one by one from the external usage like in "java.base" here, then in Swing, then in 2D like fonts, then last in AWT.

That change should not break something other than the tests which intentionally create different appcontexts. And it's easy to review.

@mrserb
Copy link
Member Author

@mrserb mrserb commented Sep 17, 2021

No objection to removing this legacy/unused code but I think it would be useful to useful to have the JBS issue or the PR summary provide a bit more context. As I see it, this is just one piece of the overall cleanup and I assume there are more substantive changes to the java.desktop module coming, is that right?

I have updated the PR and JBS

@mrserb
Copy link
Member Author

@mrserb mrserb commented Sep 29, 2021

Does anybody have any other suggestions?

@bridgekeeper
Copy link

@bridgekeeper bridgekeeper bot commented Oct 27, 2021

@mrserb This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 weeks passes without any activity. To avoid this, simply add a new comment to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration!

@mrserb
Copy link
Member Author

@mrserb mrserb commented Oct 29, 2021

Does anybody have any other suggestions?

@mlchung
Copy link
Member

@mlchung mlchung commented Oct 29, 2021

The change looks okay. My suggestion is to get 1-6 all ready to push around the same time. It's okay to have separate JBS issues and PRs.

@mrserb
Copy link
Member Author

@mrserb mrserb commented Nov 8, 2021

The change looks okay. My suggestion is to get 1-6 all ready to push around the same time. It's okay to have separate JBS issues and PRs.

ok, I'll continue to work using the plan from the description.

@bridgekeeper
Copy link

@bridgekeeper bridgekeeper bot commented Dec 6, 2021

@mrserb This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 weeks passes without any activity. To avoid this, simply add a new comment to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration!

@bridgekeeper
Copy link

@bridgekeeper bridgekeeper bot commented Jan 7, 2022

@mrserb This pull request has been inactive for more than 8 weeks and will now be automatically closed. If you would like to continue working on this pull request in the future, feel free to reopen it! This can be done using the /open pull request command.

@bridgekeeper bridgekeeper bot closed this Jan 7, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
awt core-libs rfr security
5 participants