[jira] [Commented] (FLINK-18815) AbstractCloseableRegistryTest.testClose unstable

["Kezhu Wang (Jira)" <ji...@apache.org>]

    [ https://issues.apache.org/jira/browse/FLINK-18815?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17173926#comment-17173926 ] 

Kezhu Wang commented on FLINK-18815:
------------------------------------

I think this is caused by {{SafetyNetCloseableRegistry.PhantomDelegatingCloseableRef#close}}. It unconditionally closes underlying {{innerCloseable}} without checking whether it was closed or unregistered before. Suppose that:
 # At some point, {{innerCloseable}}'s wrapping closeable was finalized and become phantom reference, its phantom reference was pushed to {{SafetyNetCloseableRegistry#REAPER_THREAD#referenceQueue}}.
 # The closeable registry containing {{innerCloseable}} was closed thus {{innerCloseable}} got closed and {{AbstractCloseableRegistry#closeableToRef}} got cleared.

 # {{SafetyNetCloseableRegistry#REAPER_THREAD}} executes {{SafetyNetCloseableRegistry.PhantomDelegatingCloseableRef#close}} which close {{innerCloseable}} unconditionally.

If we change {{Thread.sleep(2)}} from {{AbstractCloseableRegistryTest.ProducerThread}} to {{Thread.sleep(0)}}, then {{AbstractCloseableRegistryTest#testClose}} fails quite often.

Though {{Closeable#close}} declares itself be idempotent, but it does not specify any thread-safe guarantee, so I think this should be treated as bug and thus need to fix.

I think this could be fixed by closing {{SafetyNetCloseableRegistry.PhantomDelegatingCloseableRef#innerCloseable}} iff pair of {{innerCloseable}} and {{PhantomDelegatingCloseableRef}} was successfully from {{AbstractCloseableRegistry#closeableToRef}}.

I think this should be sufficient to fix concurrent removing/closing of {{Closeable}} in {{SafetyNetCloseableRegistry}} due to following observation:
 # There are only four removing paths: {{SafetyNetCloseableRegistry#close}}, {{ClosingFSDataInputStream#close}}, {{SafetyNetCloseableRegistry#unregisterCloseable}} and {{PhantomDelegatingCloseableRef#close}}.
 # I think the first three paths by design should have no concurrent contention.
 # {{PhantomDelegatingCloseableRef#close}} has no concurrent contention with {{SafetyNetCloseableRegistry#unregisterCloseable}} and {{ClosingFSDataInputStream#close}} since it is phantom reference of later removing/closing stream.

[~rmetzger] [~dianfu] [~aljoscha] [~srichter] Any opinions on this ? If we tend to fix this, could I take over it ?

> AbstractCloseableRegistryTest.testClose unstable
> ------------------------------------------------
>
>                 Key: FLINK-18815
>                 URL: https://issues.apache.org/jira/browse/FLINK-18815
>             Project: Flink
>          Issue Type: Bug
>          Components: FileSystems, Tests
>    Affects Versions: 1.12.0
>            Reporter: Robert Metzger
>            Priority: Major
>              Labels: test-stability
>
> https://dev.azure.com/apache-flink/apache-flink/_build/results?buildId=5164&view=logs&j=0da23115-68bb-5dcd-192c-bd4c8adebde1&t=05b74a19-4ee4-5036-c46f-ada307df6cf0
> {code}
> [ERROR] Tests run: 6, Failures: 1, Errors: 0, Skipped: 0, Time elapsed: 1.509 s <<< FAILURE! - in org.apache.flink.core.fs.SafetyNetCloseableRegistryTest
> [ERROR] testClose(org.apache.flink.core.fs.SafetyNetCloseableRegistryTest)  Time elapsed: 1.15 s  <<< FAILURE!
> java.lang.AssertionError: expected:<0> but was:<-1>
> 	at org.junit.Assert.fail(Assert.java:88)
> 	at org.junit.Assert.failNotEquals(Assert.java:834)
> 	at org.junit.Assert.assertEquals(Assert.java:645)
> 	at org.junit.Assert.assertEquals(Assert.java:631)
> 	at org.apache.flink.core.fs.AbstractCloseableRegistryTest.testClose(AbstractCloseableRegistryTest.java:93)
> 	at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
> {code}



--
This message was sent by Atlassian Jira
(v8.3.4#803005)