spark-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From GitBox <...@apache.org>
Subject [GitHub] [spark] zhli1142015 commented on a change in pull request #28769: [SPARK-31929][WEBUI] Close leveldbiterator when leveldb.close
Date Fri, 12 Jun 2020 05:44:51 GMT

zhli1142015 commented on a change in pull request #28769:
URL: https://github.com/apache/spark/pull/28769#discussion_r439218088



##########
File path: common/kvstore/src/main/java/org/apache/spark/util/kvstore/LevelDB.java
##########
@@ -247,17 +266,28 @@ public void close() throws IOException {
     }
   }
 
+  public boolean isClosed() {
+    return this._db.get() == null;
+  }
+
   /**
    * Closes the given iterator if the DB is still open. Trying to close a JNI LevelDB handle
    * with a closed DB can cause JVM crashes, so this ensures that situation does not happen.
    */
-  void closeIterator(LevelDBIterator<?> it) throws IOException {
+  public void closeIterator(DBIterator it) throws IOException {
     synchronized (this._db) {
       DB _db = this._db.get();
       if (_db != null) {
         it.close();
       }
     }
+    iteratorTracker.remove(it);
+  }
+
+  public DBIterator createIterator() {

Review comment:
       
   
   
   > Yes, so why track DBIterator separately?
   > If an iterator leaks, previously, it might be closed by finalize(). 
   
   If an iterator leaks (i mean after db is closed), it would not be closed any more. Here
is the cause: If level db is closed, access to related JNI handle ( 
   calling `DBIterator.close()` here ) would trigger JVM crashes. This is also the reason
that we need check if db is closed in `LevelDBcloseIterator`.
   ![4](https://user-images.githubusercontent.com/10524738/84468475-7f1cbc80-acb1-11ea-8dde-5b3775463fbb.jpg)
   This means if db is closed, then `LevelDBIterator.finalize()` will not close `DBIterator`.
`DBIterator` and related JNI resource will be leaked till restarting.
    Yes, `DBIterator` are held, if DB is opend, `LevelDBIterator.finalize()` or `LevelDBIterator.close`
would close and release them. otherwise they would be closed in `LevelDB.close`. This makes
sure they can be closed and released in time.
   
   > If you hold a strong ref to all iterators, they can't be GCed. A soft reference would
allow it. If the reference is null, that's fine, it means it was GCed and thus finalized and
closed. Weak refs won't work here, yes.
   
   About soft reference, I think it has same issue with weak reference: it's value ( `get()`
) is marked as null before `finalize` get executed, this behavior is same with weak reference.
maybe not too often, but it's still has chance encounter the race condition i mentioned above.

   
   




----------------------------------------------------------------
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:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org


Mime
View raw message