On Wed, 6 Dec 2023 19:20:22 GMT, Phil Race <p...@openjdk.org> wrote:
>> Anton Bobrov has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> 8321176: remove RuntimeException and address review comments
>
> src/java.desktop/unix/native/libawt_xawt/awt/screencast_pipewire.c line 102:
>
>> 100: if (screenProps->data->stream) {
>> 101: fp_pw_thread_loop_lock(pw.loop);
>> 102: fp_pw_stream_disconnect(screenProps->data->stream);
>
> I had this disconnect inside the lock in an earlier PR
> https://github.com/openjdk/jdk/pull/14428/files?diff=unified&w=0#diff-3ba5df79cdd3da36b21bf29b4e8de462dd61e6a21dfe0816e4d84c18bbfb76b2R92
> but then it was moved out of the lock in another PR to fix a bug
> https://bugs.openjdk.org/browse/JDK-8310334
> https://github.com/openjdk/jdk/pull/15250/files#diff-3ba5df79cdd3da36b21bf29b4e8de462dd61e6a21dfe0816e4d84c18bbfb76b2
>
>
> So how can it be right to put it back inside the lock ?
@prrace the related previous fixes didnt really fix anything and just
re-shuffled things around to dodge specific problems encountered. the
libpipewire API locking and signaling has been fundamentally broken before my
patch for JDK-8320655. this is understandable given libpipewire lack of proper
documentation in that area and no clear specs on when and how locking and
signaling should be done.
this particular case you refer to you need to lock around disconnect bc if you
don't there is a chance the disconnect API can fail here
https://github.com/PipeWire/pipewire/blob/master/src/pipewire/thread-loop.c#L100
since this is recursive lock, taking it explicitly before calling disconnect
ensures this does not happen. if you test the old implementation with
libpipewire debug enabled you would see it constantly failing in related API
and complaining functions being called in the "wrong context", including this
code in question.
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/16978#discussion_r1417918800
