-
Notifications
You must be signed in to change notification settings - Fork 843
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
NbClipboard adjustments: Avoid blocking EDT, and ease retries #7668
base: master
Are you sure you want to change the base?
Conversation
…reating a lot of logging messages, and clipboard access is a heavy operation that can sometimes interfere with other applications.
…tch Thread, as they may block while the expensive getContents() method is being called. In cases where addFlavorListener may previously have blocked for some time, the subsequent call to scheduleGetFromSystemClipboard would have been correspondingly delayed. This may or may not be of significance. Note that the call to activateWindowHack(false) has no effect; it just sets lastWindowActivated, which is not used anywhere.
The fields lastWindowActivated, lastWindowDeactivated, and lastWindowDeactivatedSource, and the related conditions were no longer in actual use; this can be seen by reading through the code. It seems the condition that once used lastWindowActivated was at some point replaced by a call to Task.waitFinished. So there wasn't a mistake; just some unused code left around. See the relevant commit from 2012: eirikbakke/netbeans-releases@6de619c#diff-69d67e2e5b062ae995069c62294958e6e24a42c599cc6684c47f89b086c20278
} | ||
Thread.sleep(20); // Give system time to settle | ||
Thread.sleep(delay); // Give system time to settle | ||
delay *= 2; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Am I missing something here?! If the delay is multiplied by two each iteration, how do we get anywhere near MAX_TRIES
before timing out?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Correct, we can never reach MAX_RETRIES. There can be max 7 attempts if getContents fails immediately: 0+20+40+80+160+320+640 = 1260 milliseconds and then the "> 1000" condition breaks out of the loop the next time it fails). I could get rid of MAX_RETRIES without changing the behavior here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Glad our maths matches! I would just make sure the values match up in logic so it doesn't confuse the next person - eg. make it MAX_RETRIES = 7
? Just in case you hit that one moment the system clock changes and this keeps trying for hours! 😆
Commits 58312a4 and fb59d75 make sense to me. The commit "Avoid calling Clipboard.add/removeFlavorListener from the Event Dispatch Thread [...]" gives me bad vibes 8c0e3d7188e023dfcfbbd79c07dc9b0f62f5f7083). The one mantra in Swing I learned was: You interact with Swing/AWT only from the EDT, unless it is explicitly documented, that you can deviate from this rule. I don't see that exception in the |
@matthiasblaesing Yeah, it's not officially documented in the Javadoc for the java.awt.datatransfer.Clipboard class, but reading through its source code, it's clear that it is intended to be thread-safe, and NetBeans already assumes this to be the case (see the GetContents and SetContents tasks, which are run on a separate RequestProcessor thread). The class does I/O operations that may block for several seconds, so it can't be used purely on the event dispatch thread without regularly locking up the UI. In the Clipboard.addFlavorListener/removeFlavorListener case, you can call either method from any thread, but the actual events will be delivered on the Event Dispatch Thread. The fact that addFlavorListener/removeFlavorListener can block for several seconds (due to lock contention with the I/O-heavy getContent operation) was almost certainly unintentional, but calling them off-thread avoids the problem of locking up the UI. Reviewing the logic again, I can try to justify why the proposed code is safe...
|
@eirikbakke I'm aware, that there is already a threading violation in NbClipboard. My concern is, that that is the reason for #3962. And my concern is also not what happens on the NetBeans side of the Clipboard, but what happens on the way from NetBeans layer, through AWT to native. In the end I saw a window message loop inside AWT and if I remember correctly I diagnosed a problem in AWT once that related to the threading assumption of the COM calls inside the AWT. |
@matthiasblaesing It's quite possible (likely, even) that #3962 has race conditions involved. Someone would have to spend a week(end) finding the root cause of it before we could figure the best way to solve it, though. For me it was never worth it, because the previous patches, or just other environmental conditions, made the problem rare and easy to work around. Though often I find that fixing smaller understandable bugs, like the blocking removeFlavorListener() here, sometimes magically ends up fixing bigger hard-to-reproduce bugs. For the systemClipboard.addFlavorListener()/removeFlavorListener() calls it is (somewhat) easy to reason about concurrency behavior by reading the source code of java.awt.datatransfer.Clipboard class. For getContents/setContents things go much deeper, as you mention. Since I have now switched permanently from Windows to MacOS on my working machine, it is hard for me to properly test further changes to this PR, as these clipboard bugs usually come only after several days of active use. If preferred, I could drop the controversial "Avoid calling Clipboard.add/removeFlavorListener from the Event Dispatch Thread" commit and leave the two others. |
I think this would be a good idea. Thank you. |
Some improvements to the NbClipboard class, in three separate commits:
(This commit I have been using in my working IDE and platform app since January 2024.)
Event dispatch thread (AWT-EventQueue):
Clipboard synchronizer thread concurrently holding the lock:
(This commit I have been using in my working IDE on Windows for only 5 days, but I haven't seen any problems with it so far. I'll be switching to a new MacBook laptop now, so my Windows setup won't see too much organic usage after this.)
There are some long-standing intermittent bugs with Cut and Paste on Windows, which are either caused by, or have to be fixed with, changes in the NbClipboard class. The changes above were not specifically made to fix those bugs, but improving the code is probably a good thing in any case.