kezhuw commented on a change in pull request #13124:
URL: https://github.com/apache/flink/pull/13124#discussion_r469085129
##########
File path:
flink-core/src/main/java/org/apache/flink/util/AbstractCloseableRegistry.java
##########
@@ -163,9 +163,9 @@ protected final void addCloseableInternal(Closeable
closeable, T metaData) {
/**
* Removes a mapping from the registry map, respecting locking.
*/
- protected final void removeCloseableInternal(Closeable closeable) {
+ protected final boolean removeCloseableInternal(Closeable closeable, T
object) {
synchronized (getSynchronizationLock()) {
- closeableToRef.remove(closeable);
+ return closeableToRef.remove(closeable, object);
Review comment:
@tillrohrmann It is a paranoid guard against un-register and
re-register. After re-registered, there will be a different phantom reference
take over this one. Ideally, after unregistered, the phantom reference itself
is unreachable from user path, thus that phantom reference should not be
enqueued, but I am not that sure about this. If this is not going to happen,
`boolean removeCloseableInternal(Closeable closeable)` should be sufficient.
How about adding new tests to test `un-register`/`re-register` and using
`closeableToRef.remove(closeable)` ?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
[email protected]
