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

8263311: Watch registry changes for remote printers update instead of polling #2915

Closed
wants to merge 1 commit into from

Conversation

@aivanov-jdk
Copy link
Member

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

JDK-8153732 implemented polling for remote printers.
That bug description also mentions watching the registry for changes and links to the article which describes the method yet it does so in terms of WMI. Using WMI is not necessary to watch for the registry updates.

It is possible to replace polling mechanism with registry change notifications. If the registry at HKCU\Printers\Connections is updated, refresh the list of print services.

It works perfectly well in my own testing with sharing a Generic / Text Only printer from another laptop. The notification comes as soon as the printer is installed, it results in a new key created under Connections. If a remote printer is removed, the notification is also triggered as the key corresponding to that printer is removed from the registry.

I updated the steps in the manual test: RemotePrinterStatusRefresh.java.


Progress

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

Issue

  • JDK-8263311: Watch registry changes for remote printers update instead of polling

Reviewers

Download

$ git fetch https://git.openjdk.java.net/jdk pull/2915/head:pull/2915
$ git checkout pull/2915

@bridgekeeper
Copy link

@bridgekeeper bridgekeeper bot commented Mar 10, 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 10, 2021
@openjdk
Copy link

@openjdk openjdk bot commented Mar 10, 2021

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

  • 2d
  • awt

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.

@mlbridge
Copy link

@mlbridge mlbridge bot commented Mar 10, 2021

Webrevs

ERROR_SUCCESS == RegNotifyChangeKeyValue(hKey, TRUE,
REG_NOTIFY_CHANGE_NAME,
NULL,
FALSE);
Copy link
Member Author

@aivanov-jdk aivanov-jdk Mar 11, 2021

Choose a reason for hiding this comment

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

RegNotifyChangeKeyValue notifies the caller about changes to the attributes or contents of a specified registry key.

hKey: A handle to HKEY_CURRENT_USER\Printers\Connections key which is opened above.
bWatchSubtree = TRUE: The function reports changes in the specified key and its subkeys.
dwNotifyFilter = REG_NOTIFY_CHANGE_NAME: Notify the caller if a subkey is added or deleted.
hEvent = NULL: If fAsynchronous is FALSE, hEvent is ignored.
fAsynchronous = FALSE: The function does not return until a change has occurred.

When a new remote printer is added, a new key is created under HKCU\Printers\Connections; when an existing remote printer is removed, the key below Connections is removed; no values are added or removed in Connections key, thus REG_NOTIFY_CHANGE_LAST_SET filter is not needed.

Copy link
Contributor

@prsadhuk prsadhuk Mar 12, 2021

Choose a reason for hiding this comment

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

Is this only about addition/removal? What about printer name change? Shouldn't we get notified in that case as trying to print on printer with old name will not find the printer!!
If yes, in that regard I guess REG_NOTIFY_CHANGE_LAST_SET is the one to go for as it states
"Notify the caller of changes to a value of the key. This can include adding or deleting a value, or changing an existing value. "

Copy link
Member Author

@aivanov-jdk aivanov-jdk Mar 12, 2021

Choose a reason for hiding this comment

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

Is this only about addition/removal? What about printer name change?

You cannot change the name of a remote printer.

Opening Printer Properties dialog from the printer context menu on the local host and editing its name changes the name of the printer on the remote host which shares it. Windows warns, “This is a shared printer. If you rename a shared printer, existing connections to this printer from other computers will break and will have to be created again.”

Nothing has been updated on the local system after renaming the printer. As the warning said, the printer does not work any more because it refers to a printer that does not exist.

To fix this, one has to add a new remote printer which refers to the new name of the printer on the remote host.

Shouldn't we get notified in that case as trying to print on printer with old name will not find the printer!!

I'm afraid there's nothing Java can do to mitigate renaming the printer.

If yes, in that regard I guess REG_NOTIFY_CHANGE_LAST_SET is the one to go for as it states
"Notify the caller of changes to a value of the key. This can include adding or deleting a value, or changing an existing value. "

Since no values are created, removed, or changed when a remote printer is added or removed under Connections key, adding REG_NOTIFY_CHANGE_LAST_SET to dwNotifyFilter is not required. There are some values stored in the printer key but Java does not use them directly; if any value is changed there, getting notifications about such an event only creates noise.

So, as far as Java is concerned, getting notifications about new keys being created or removed under Connections is enough. This is what REG_NOTIFY_CHANGE_NAME filter does.

Copy link
Contributor

@prsadhuk prsadhuk Mar 12, 2021

Choose a reason for hiding this comment

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

I can understand that as a user, you cannot /shouldn't change the name of a remote network printer but a network admin can change the name, so shouldn't we get notification on that name change when this method gets called after the name change? or would it be notified as a new printer in that case? Then what will happen to the old stale printer in the registry?

Copy link
Member Author

@aivanov-jdk aivanov-jdk Mar 12, 2021

Choose a reason for hiding this comment

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

You can't. Try it yourself. The printer connection is per-user, after all it's stored under HKCU.

Windows displays the name of remote printers as “Generic / Text Only on 192.168.1.18”: the name of the printer and the remote host.

If you open the Printer Properties dialog of a remote printer and edit its name, you change the name of the printer on the remote host which shares it. It changes absolutely nothing on the local system.

shouldn't we get notification on that name change when this method gets called after the name change?

Yes, we should. But we cannot.

For Windows, nothing has changed on the local system, it's the remote system that has been changed.

or would it be notified as a new printer in that case?

No, it wouldn't because nothing has changed on the local system.

Then what will happen to the old stale printer in the registry?

Nothing, again. It just stays there but Windows reports an error if you try to open its Printer Properties, Windows reports an error if you try to print to it. I tried printing with Java and Notepad: the behaviour is the same. The difference is that Notepad displays the error message whereas Java throws an exception (it depends on the Java app, I guess).

The behaviour aligns to the one seen when the printer is unreachable because of, let's say, network issues.

Copy link
Contributor

@prsadhuk prsadhuk Mar 12, 2021

Choose a reason for hiding this comment

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

OK.

@openjdk
Copy link

@openjdk openjdk bot commented Mar 12, 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:

8263311: Watch registry changes for remote printers update instead of polling

Reviewed-by: psadhukhan, serb

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 50 new commits pushed to the master branch:

  • da9ead5: 8263399: CDS should archive only classes allowed by module system
  • 9c84899: 8263555: use driver-mode to run ClassFileInstaller
  • 8e562d2: 8263477: serviceability/sa/ClhsdbDumpheap.java timed out
  • a7aba2b: 8263549: 8263412 can cause jtreg testlibrary split
  • d339320: 8263136: C4530 was reported from VS 2019 at access bridge
  • a528771: 8262491: AArch64: CPU description should contain compatible board list
  • 86e4c75: 8256156: JFR: Allow 'jfr' tool to show metadata without a recording
  • 0b68ced: 8263548: runtime/cds/appcds/SharedRegionAlignmentTest.java fails to compile after JDK-8263412
  • 43524cc: 8243455: Many SA tests can fail due to trying to get the stack trace of an active method
  • e834f99: 8263412: ClassFileInstaller can't be used by classes outside of default package
  • ... and 40 more: https://git.openjdk.java.net/jdk/compare/fab567666ebb9cafbd86b536eb43164dde75579f...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 12, 2021
@mrserb
Copy link
Member

@mrserb mrserb commented Mar 13, 2021

@aivanov-jdk can you please check what that old comment was about:

https://bugs.openjdk.java.net/browse/JDK-8153732?focusedCommentId=14188974&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-14188974

Any idea why RegistryValueChange was rejected as a solution?

@aivanov-jdk
Copy link
Member Author

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

Any idea why RegistryValueChange was rejected as a solution?

I've no idea. I read that comment, it's exactly the comment that was in the code above RemotePrinterChangeListener class in PrintServiceLookupProvider.java.

RegistryValueChange cannot be used in combination with WMI to get registry value change notification because of an error that may be generated because the scope of the query would be too big to handle(at times).

It's very vague. I don't know what it really means. Neither do I know if a prototype was even tried.

If fact, this comment cites portions of the article How to listen for Printer Connections?. The link to this article is in the description of JDK-8153732.

The article discusses the subject with WMI and WQL. This is what it says:

There is no equivalent to FindFirstPrinterChangeNotification, which listens for new/changed Printer Connections – and a polling mechanism was not an option. However, there is another way, using WMI.

Note: polling mechanism was not an option. Yet it is what was implemented in Java.

Then the articles describes the steps needed to monitor for printer changes.

Close to the end, there's the quoted sentence:

Unfortunately, we cannot use the RegistryValueChangeEvent (because the scope of the query would be too big (we’d receive an error message), so we can only know when something changed below the Printers\Connections key, but not what. This is why we have to rely on EnumPrinters.

This statement is a bit confusing. It says they cannot use RegistryValueChangeEvent (it's not a Windows API function, it's another WMI class) but the WQL query above uses it.

I interpret the following statement this way: Registry notifications do not provide information on what changed under Printers\Connections, only that a change occurred. So EnumPrinters is used to enumerate the remote printers and update the stored list of printers after the change to the registry occurs.

I used this article as the inspiration and the implementation I'm proposing in this PR is purely based on this article. Yet I'm using Windows API function RegNotifyChangeKeyValue to watch for registry updates and to get notified as soon as a change under HKCU\Printers\Connections occurs. Then the list of printers is updated by the refreshServices upcall into Java which refreshes both local and remote printers.

mrserb
mrserb approved these changes Mar 14, 2021
@aivanov-jdk
Copy link
Member Author

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

/integrate

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

@openjdk openjdk bot commented Mar 18, 2021

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

  • 3f31a6b: 8263775: C2: igv_print() crash unexpectedly when called from debugger
  • 63eae8f: 8260605: Various java.lang.invoke cleanups
  • 9cd21b6: 8263590: Rawtypes warnings should be produced for pattern matching in instanceof
  • ff52f29: 8260716: Assert in MacroAssembler::clear_mem with -XX:-IdealizeClearArrayNode
  • 72b82fd: 8263725: JFR oldobject tests are not run when GCs are specified explicitly
  • 444a80b: 8263455: NMT: assert on registering a region which completely engulfs an existing region
  • 2b93ae0: 8261480: MetaspaceShared::preload_and_dump should check exceptions
  • 81ba578: 8263676: AArch64: one potential bug in C1 LIRGenerator::generate_address()
  • 9225a23: 8263108: Class initialization deadlock in java.lang.constant
  • 5d5813a: 8263757: Remove serviceability/sa/ClhsdClasses.java from ZGC problem list
  • ... and 118 more: https://git.openjdk.java.net/jdk/compare/fab567666ebb9cafbd86b536eb43164dde75579f...master

Your commit was automatically rebased without conflicts.

Pushed as commit a85dc55.

💡 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-8263311 branch Mar 18, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
3 participants