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 Looks good to me. > can you please file a bug based on my description above ? Filed the [JDK-8321475](https://bugs.openjdk.org/browse/JDK-8321475) for this > and also something i thought i should mention wrt doCleanup() function. it > might be a good idea to protect it with a mutex or something. it does look > safe ATM but i haven't looked at related libpipewire internals to assert that > with full certainty. there perhaps could be a situation where doCleanup() is > called internally but also from java cleanup thread (the one that sleeps for > 2 seconds) and if they interleave/race it might cause unexpected behavior or > crash in libpipewire as its rather sensible in terms of teardown sequence > (i've seen its crashing due to that in my testing and thats why i have > reshuffled some related calls in this patch). anyways, this is something to > look into and check. Sure, we should revisit it later. ------------- Marked as reviewed by azvegint (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/16978#pullrequestreview-1768293452
