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

8263984: Invalidate printServices when there are no printers #3151

Closed
wants to merge 3 commits into from

Conversation

@aivanov-jdk
Copy link
Member

@aivanov-jdk aivanov-jdk commented Mar 23, 2021

When getAllPrinterNames() returns null, the list of printServices is assigned a new empty array without invalidating old services which were in the array before.

The old print services should be invalidated.


Progress

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

Issue

  • JDK-8263984: Invalidate printServices when there are no printers

Reviewers

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 3151

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

Using diff file

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

@bridgekeeper
Copy link

@bridgekeeper bridgekeeper bot commented Mar 23, 2021

👋 Welcome back aivanov! 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 openjdk bot added the rfr label Mar 23, 2021
@openjdk
Copy link

@openjdk openjdk bot commented Mar 23, 2021

@aivanov-jdk The following label will be automatically applied to this pull request:

  • 2d

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

@openjdk openjdk bot added the 2d label Mar 23, 2021
@mlbridge
Copy link

@mlbridge mlbridge bot commented Mar 23, 2021

Webrevs

@@ -116,6 +116,7 @@ private synchronized void refreshServices() {
if (printers == null) {
Copy link
Member Author

@aivanov-jdk aivanov-jdk Mar 23, 2021

Choose a reason for hiding this comment

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

getAllPrinterNames returns null when an error occurs or when there are no printers in the system.

Up for discussion:
If an error occurs, we may leave the old services without invalidating them. In this case, it is necessary to distinguish between the error and the lack of printers in the system. Does this make any sense?

@@ -116,6 +116,7 @@ private synchronized void refreshServices() {
if (printers == null) {
// In Windows it is safe to assume no default if printers == null so we
// don't get the default.
invalidateServices();
Copy link
Member Author

@aivanov-jdk aivanov-jdk Mar 23, 2021

Choose a reason for hiding this comment

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

The default print service, defaultPrintService, is usually in printServices array. It's possible that it's not in the array; in this case getDefaultPrintService() will reset it to null when accessed.

@@ -116,6 +116,7 @@ private synchronized void refreshServices() {
if (printers == null) {
// In Windows it is safe to assume no default if printers == null so we
// don't get the default.
invalidateServices();
printServices = new PrintService[0];
Copy link
Contributor

@prsadhuk prsadhuk Mar 24, 2021

Choose a reason for hiding this comment

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

I am not sure this call to invalidateServices() is needed as we are creating a new printServices object of 0 length so making the old printServices invalid is redundant as it is same global variable.

Copy link
Member Author

@aivanov-jdk aivanov-jdk Mar 24, 2021

Choose a reason for hiding this comment

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

That's exactly the problem I am addressing.

In the regular case, the existing services left in printServices are invalidated before newServices is assigned to it. Yet no existing services are invalidated if all the printers were removed from the system.

Why is it needed to invalidate deleted services if the list of printers is updated and if at least one printer is left in the system? Why is it not needed to invalidate deleted services if all the printers are removed from the system?

Copy link
Contributor

@prsadhuk prsadhuk Mar 24, 2021

Choose a reason for hiding this comment

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

Since this is windows specific code, I am not sure if system will not have any printers. I guess by default, Microsoft XPS Document Writer, Microsoft Print-to-PDF, Fax are present in printers list. That is the reason we do not have getPrintService() return 0 printer although there may not be any real printer present in windows system...reason for some failure in jtreg test which caused us to use @key printer tag in those tests to make real printers are configured.

Copy link
Member Author

@aivanov-jdk aivanov-jdk Mar 24, 2021

Choose a reason for hiding this comment

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

Since this is windows specific code, I am not sure if system will not have any printers. I guess by default, Microsoft XPS Document Writer, Microsoft Print-to-PDF, Fax are present in printers list.

Yes, that's correct. But you can remove them. You can also configure the image so that these are not installed by default.

That is the reason we do not have getPrintService() return 0 printer although there may not be any real printer present in windows system...reason for some failure in jtreg test which caused us to use @key printer tag in those tests to make real printers are configured.

I agree that getting zero printers in Windows is unlikely. But it's still possible.

Consider the following scenario: Let's assume there are 10 printers in the system. The user removes 5 of them. In this case, invalidateService() is called on the instances of Win32PrintService which were removed from the system.

Then the user removes the remaining 5 printers. In this case, invalidateService() is not called at all. If an application has references to any of these instances, they will continue to appear operational. However, the flag isInvalid in Win32PrintService is used in two methods only: getPrinterState and getPrinterStateReasons.

This fix is minor, probably this situation never occurs in the real life.

The difference in handling the deleted services caught my attention. If everyone agrees it's not problem, I'll withdraw the PR and close the bug as Not an Issue.

Copy link
Member

@jayathirthrao jayathirthrao Apr 1, 2021

Choose a reason for hiding this comment

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

Change looks like good practice and needed, eventhough it may occur rarely.

mrserb
mrserb approved these changes Mar 26, 2021
@openjdk
Copy link

@openjdk openjdk bot commented Mar 26, 2021

@aivanov-jdk This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

8263984: Invalidate printServices when there are no printers

Reviewed-by: serb, jdv

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been 233 new commits pushed to the master branch:

  • c3abdc9: 8264797: Do not include klassVtable.hpp from instanceKlass.hpp
  • eb5c097: 8262389: Use permitted_enctypes if default_tkt_enctypes or default_tgs_enctypes is not present
  • bfb034a: 8264524: jdk/internal/platform/docker/TestDockerMemoryMetrics.java fails due to swapping not working
  • a756d8d: 8264759: x86_32 Minimal VM build failure after JDK-8262355
  • 0f13e22: 8264791: java/util/Random/RandomTestBsi1999.java failed "java.security.SecureRandom nextFloat consecutive"
  • 4bb80f3: 8262898: com/sun/net/httpserver/bugs/8199849/ParamTest.java times out
  • 2f51699: 8264554: X509KeyManagerImpl calls getProtectionParameter with incorrect alias
  • 114e3c3: 8263856: Github Actions for macos/aarch64 cross-build
  • a611c46: 8264048: Fix caching in Jar URL connections when an entry is missing
  • bf26a25: 8264027: Refactor "CLEANUP" region printing
  • ... and 223 more: https://git.openjdk.java.net/jdk/compare/b2df51372ff960cddd2191a496e742e6c150e3d4...master

As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.

➡️ To integrate this PR with the above commit message to the master branch, type /integrate in a new comment.

@openjdk openjdk bot added the ready label Mar 26, 2021
// Look for deleted services and invalidate these
if (printServices != null) {
for (int j=0; j < printServices.length; j++) {
if ((printServices[j] instanceof Win32PrintService) &&
(!printServices[j].equals(defaultPrintService))) {
(!printServices[j].equals(defaultPrintService))) {
Copy link
Member

@jayathirthrao jayathirthrao Apr 1, 2021

Choose a reason for hiding this comment

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

Is this indentation right? I thought we always map newline additional conditions and not add extra indentation.

Copy link
Member Author

@aivanov-jdk aivanov-jdk Apr 1, 2021

Choose a reason for hiding this comment

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

Is it not right?
I admit I don't understand what you mean by map here.

When the code is written like it was:

                if ((printServices[j] instanceof Win32PrintService) &&
                    (!printServices[j].equals(defaultPrintService))) {
                    ((Win32PrintService)printServices[j]).invalidateService();
                }

it's hard to scan: it's not clear what is part of the condition and what is the statement inside the if block.

I'd prefer to write it like this:

                if ((printServices[j] instanceof Win32PrintService)
                    && (!printServices[j].equals(defaultPrintService))) {

                    ((Win32PrintService)printServices[j]).invalidateService();
                }

That is moving the operator to the continuation line which makes it obvious it is a continuation line of the condition and adding a blank line before the statement in the code. It's still not perfect, however; and it changes the author in blame output.

I indented the continuation line by additional 8 spaces, which is also a common practice in Java, to visually separate the condition and the statement. In fact, it's IDE that updated the formatting, I decided to keep it because it makes the code clearer.

I can revert the change to this line if you like.
Or I can just add a blank line between the condition and the statement.

What is your preference?

Copy link
Member

@jayathirthrao jayathirthrao Apr 1, 2021

Choose a reason for hiding this comment

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

By mapping i mean same indentation for all conditions in if statement without adding additional indentation for each continuation line like(Basically line without your change of indentation)

if ((condition1) &&
     (condition2)) {
}

or

if ((condition1)
     &&(condition2)) {
}

I have not come across code in java.desktop where we add indentation at each continuation line of 'if' condition.
I understand difficulty to scan without indentation but then in cases where we have multiple lines on continuation line in if statement we will easily hit 80 characters limit.

If we want to differentiate between if conditions and actual statement execution to improve readability, we can move the statement block to new line like

if ((condition1) &&
     (condition2))
{
}

Copy link
Member

@jayathirthrao jayathirthrao Apr 1, 2021

Choose a reason for hiding this comment

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

I would prefer if you revert this line or if we want to put emphasis on readability moving '{' to new line also seems fine.

Copy link
Member Author

@aivanov-jdk aivanov-jdk Apr 1, 2021

Choose a reason for hiding this comment

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

By mapping i mean same indentation for all conditions in if statement…

Okay, it's the first time I've come across to such a use of map.

I have not come across code in java.desktop where we add indentation at each continuation line of 'if' condition.

Take a look at DefaultTableCellRenderer.getTableCellRendererComponent.

I understand difficulty to scan without indentation but then in cases where we have multiple lines on continuation line in if statement we will easily hit 80 characters limit.

What is more important code readability or the strict limit of 80 columns?

All in all, all these styles are used throughout java.desktop module.
I chose to opt for readability in this particular case and the line fits into 80 column limit.

Do I revert the change to this line?
Any other suggestions? What is your preference?

Copy link
Member

@mrserb mrserb Apr 2, 2021

Choose a reason for hiding this comment

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

BTW another solution is to move { to the next line if the previous statement was split across few lines.

Copy link
Member Author

@aivanov-jdk aivanov-jdk Apr 6, 2021

Choose a reason for hiding this comment

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

I added a blank line between the condition and the body.
Does it look good? @mrserb, @jayathirthrao

Copy link
Member Author

@aivanov-jdk aivanov-jdk Apr 6, 2021

Choose a reason for hiding this comment

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

Code Conventions for Java say, “Line wrapping for if statements should generally use the 8-space rule, since conventional (4 space) indentation makes seeing the body difficult.” (It's the second to last block on the page.)

Copy link
Contributor

@prsadhuk prsadhuk Apr 7, 2021

Choose a reason for hiding this comment

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

If we are adding a new line, then I think we should need to add at l129, l138 otherwise it will look odd doing it at one place only.

Copy link
Member Author

@aivanov-jdk aivanov-jdk Apr 7, 2021

Choose a reason for hiding this comment

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

I haven't touched that code at all.
Not that odd because it's isolated to the new function now.

What is your suggestion? Refactor all if statements in these two functions? Submit a separate bug for refactor all if statements the entire file? Revert back to no new line, leaving this particular if untouched as well?

mrserb
mrserb approved these changes Apr 7, 2021
@mlbridge
Copy link

@mlbridge mlbridge bot commented Apr 7, 2021

Mailing list message from Prasanta Sadhukhan on 2d-dev:

I have not looked at entire file but since this function was modified, I made that suggestion. I am not that particular if you wish to ignore that.

Regards
Prasanta

Get Outlook for Android<https://aka.ms/AAb9ysg>
________________________________
From: 2d-dev <2d-dev-retn at openjdk.java.net> on behalf of Alexey Ivanov <aivanov at openjdk.java.net>
Sent: Wednesday, April 7, 2021 4:22:29 PM
To: 2d-dev at openjdk.java.net <2d-dev at openjdk.java.net>
Subject: Re: [OpenJDK 2D-Dev] RFR: 8263984: Invalidate printServices when there are no printers [v3]

On Wed, 7 Apr 2021 04:12:55 GMT, Prasanta Sadhukhan <psadhukhan at openjdk.org> wrote:

[Code Conventions for Java](https://www.oracle.com/java/technologies/javase/codeconventions-indentation.html#248) say, ?Line wrapping for `if` statements should generally use the 8-space rule, since conventional (4 space) indentation makes seeing the body difficult.? (It's the second to last block on the page.)

If we are adding a new line, then I think we should need to add at l129, l138 otherwise it will look odd doing it at one place only.

I haven't touched that code at all.
Not that odd because it's isolated to the new function now.

What is your suggestion? Refactor all if statements in these two functions? Submit a separate bug for refactor all if statements the entire file? Revert back to no new line, leaving this particular if untouched as well?

-------------

PR: https://git.openjdk.java.net/jdk/pull/3151
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.openjdk.java.net/pipermail/2d-dev/attachments/20210407/e4f5bb85/attachment.htm>

@aivanov-jdk
Copy link
Member Author

@aivanov-jdk aivanov-jdk commented Apr 7, 2021

/integrate

@openjdk openjdk bot closed this Apr 7, 2021
@openjdk openjdk bot added integrated and removed ready rfr labels Apr 7, 2021
@openjdk
Copy link

@openjdk openjdk bot commented Apr 7, 2021

@aivanov-jdk Since your change was applied there have been 237 commits pushed to the master branch:

  • adb860e: 8255800: Raster creation methods need some specification clean up
  • eab8455: 8261137: Optimization of Box nodes in uncommon_trap
  • 92fad1b: 8264680: Use the blessed modifier order in java.desktop
  • 17202c8: 8264748: Do not include arguments.hpp from compilerDefinitions.hpp
  • c3abdc9: 8264797: Do not include klassVtable.hpp from instanceKlass.hpp
  • eb5c097: 8262389: Use permitted_enctypes if default_tkt_enctypes or default_tgs_enctypes is not present
  • bfb034a: 8264524: jdk/internal/platform/docker/TestDockerMemoryMetrics.java fails due to swapping not working
  • a756d8d: 8264759: x86_32 Minimal VM build failure after JDK-8262355
  • 0f13e22: 8264791: java/util/Random/RandomTestBsi1999.java failed "java.security.SecureRandom nextFloat consecutive"
  • 4bb80f3: 8262898: com/sun/net/httpserver/bugs/8199849/ParamTest.java times out
  • ... and 227 more: https://git.openjdk.java.net/jdk/compare/b2df51372ff960cddd2191a496e742e6c150e3d4...master

Your commit was automatically rebased without conflicts.

Pushed as commit 9d65039.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

@aivanov-jdk aivanov-jdk deleted the JDK-8263984 branch Apr 7, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
4 participants