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 @azvegint thanks for review! yeah the CSR looks like a long story so lets skip it in this patch however i feel that the problem has to be addressed somehow going forward as i dont think the current behavior is acceptable. there could be cases where it can cause real problems eg imagine some sort of medical imaging app where it could fail intermittently and report wrong pixel colors etc. can you please file a bug based on my description above ? 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. ------------- PR Comment: https://git.openjdk.org/jdk/pull/16978#issuecomment-1842498910
