On Wed, 6 Dec 2023 09:08:55 GMT, Anton Bobrov <d...@openjdk.org> wrote:
>> This patch adds re-try logic to libpipewire screencast error handling as >> discussed in PR #16794 and also brings some additional error handling and >> thread safety improvements. Specifically around cleanup order where >> incorrect ordering lead to native memory corruption issues, and lock/unlock >> accounting that while mostly harmless (with the current libpipewire >> implementation) did pollute the stderr on jtreg tests, making some tests >> (expecting clean stderr) to fail. >> >> The real major change here however is the throw of the RuntimeException >> which can propagate to public >> >> java.awt.Robot. createMultiResolutionScreenCapture, createScreenCapture, >> getPixelColor methods. I'm not sure the plain RuntimeException is the way to >> go here so this is just a placeholder of sorts. A separate/specific runtime >> exception can be created for this BUT *something* needs to be done here as >> the current implementation fails to convey libpipewire failures to those >> public API callers and since they have no way of detecting such errors >> otherwise they have no way of knowing that the data returned by those API is >> in fact invalid (eg black screens etc). The reason for using an unchecked >> exception here is driven mainly by the following factors: >> >> 1) Since those are public API, their contracts can potentially make it >> difficult to introduce specific additional checked exceptions or return >> values (as appropriate) as those could potentially break existing API use. >> >> 2) The libpipewire errors of that kind are rare and usually indicate there >> is something wrong with the desktop stack eg some fatal configuration or run >> time error that is normally not supposed to happen and given this patch now >> goes extra step re-trying on such failures it stands to reason runtime >> unchecked exception makes sense when that fails as well. >> >> 3) Creating checked exceptions for such specific native implementation >> dependent errors and propagating such exceptions thru the call tree does not >> make much sense as most public API users won't even know how to handle them >> without knowing native implementation specifics. > > Anton Bobrov has updated the pull request incrementally with one additional > commit since the last revision: > > 8321176: remove RuntimeException and address review comments Marked as reviewed by prr (Reviewer). ------------- PR Review: https://git.openjdk.org/jdk/pull/16978#pullrequestreview-1773235252
