[prev in list] [next in list] [prev in thread] [next in thread] 

List:       openjdk-2d-dev
Subject:    Re: RFR: 8321176: [Screencast] make a second attempt on screencast failure [v2]
From:       Phil Race <prr () openjdk ! org>
Date:       2023-12-08 22:13:15
Message-ID: 2WLdBmTf4SEjk1-rHhPccj0aGawqDIxlDyK2a9a8q0E=.742f9c94-7011-439b-bdd4-766a65f772df () github ! com
[Download RAW message or body]

On Wed, 6 Dec 2023 09:08:55 GMT, Anton Bobrov <duke@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


[prev in list] [next in list] [prev in thread] [next in thread]