Idiom to close a cursor

Question

Which of the following two should I be using to make sure that all the cursors are closed?

    Cursor c = getCursor(); 

    if(c!=null && c.getCount()>0){ 
        try{ 
            // read values from cursor 
        }catch(..){} 
        finally{ 
            c.close(); 
        } 
    }//end if
    

OR

    Cursor c = getCursor(); 
    try{ 
        if(c!=null && c.getCount()>0){ 
            // read values from cursor 
        }//end if 
    }catch(..){
        
    }finally{ 
        c.close(); 
    } 

Please advise.

Parapet

Answer 1

Neither, but the second one was closest.

• Option 1 doesn't properly close the Cursor when getCount() == 0
• Option 2 leaves the finally block exposed to a null pointer exception

I would use:

Cursor c = getCursor(); 
try { 
    if(c!=null && c.getCount()>0){ 
         // do stuff with the cursor
    }
}
catch(..) {
    //Handle ex
}
finally { 
    if(c != null) {
        c.close(); 
    }
}

... or if you expect the cursor to be null frequently, you could turn it on its head a little bit:

Cursor c = getCursor(); 
if(c != null) {
    try { 
        if(c.getCount()>0) { 
             // do stuff with the cursor
        }
    }
    catch(..) {
        //Handle ex
    }
    finally { 
        c.close(); 
    }
}

Kailey

i don't think use getCount is a good method. if you user moveToFirst, you can get better performance – Proceeding

@Proceeding - That's a valid point moveToFirst is more performant AND answers the question of "is there anything in the result set"... but OP used getCount() in their example so I continued it here. – Kailey

Answer 2

This is even better:

• does not use c.getCount() - counting might require extra work for the database and is not needed
• initialize the cursor before the query block, so failure to create the query is not followed by the finally block

The code:

Cursor c = query(....);
if (c != null) {
   try {        
       while (c.moveToNext()) {  // If empty or after last record it returns false.    
          // process row...
       }
   } 
   finally {
       c.close();
    }
}

Note that c might be null in case of error or empty cursor. See https://mcmap.net/q/265354/-what-causes-android-39-s-contentresolver-query-to-return-null. I would report null return value in case of empty cursor as a bug, though.

Grissom

NPE is an issue, query could return null. – Cobbett

I meant no c != null check is necessary in finally. If query returns null, it will fail with NPE, the same as Hemant's snippet would. And I think the query() method will never return null. It should create the query, or throw an exception, in which case you don't need to run the finally block. This is normal cleanup pattern: create resource ... try ... do work ... finally ... clean up ... end. If creation fails, it is reported. If work fails or succeeds, finally does the cleanup. If you understand, please remove negative vote. If not, please write. – Grissom

It can return null and it's specified in the docs (see ContentResolver.query). Also, check this: #13081040 – Cobbett

Ok, but null return value is not specified. As it is mentioned in the post, it may be due to some error or due to empty cursor. I think this ambiguity is worth reporting as a bug. I'll update my answer. Thanks. – Grissom

Answer 3

Best practice is the one below:

Cursor c = null;    
try {        
   c = query(....);      
   while (c.moveToNext()) {  // If empty or next to last record it returns false.    
      // do stuff..       
   }
} finally {
   if (c != null && !c.isClosed()) {  // If cursor is empty even though should close it.       
   c.close();
   c = null;  // high chances of quick memory release.
}

Bunns

I wonder if in this case it is really best practice to set cursor to null, since it is a local variable, the GC should be enough smart to handle it right? – Ultramontane

@Ultramontane Your logic is logical, but without setting the Cursor to null, it may not be initialized when entering the finally block. You know it is serious when even Android Studio doesn't feel it needs to be explained. – Burton

Answer 4

Depends on what you're catching, but I'd say the second one, just in case c.getCount() throws an exception.

Also, some indentation wouldn't go amiss :)

Scrabble

Answer 5

I'd say the first one, mainly because the second one will try to call c.close() even if c is null. Also, according to the docs, getCount()doesn't throw any exceptions, so there's no need to include it in the try block.

Chat

Do we need to call close() on a cursor which has count of 0? Because in that case for the first idiom, close() will never be called. It assumes that for a cursor having no elements, cursor will never be opened. Is this a valid assumption? – Parapet

No. A Cursor needs to be closed no matter how many items it has. – Chat

You can just skip the c.getCount() > 0 condition. This way, your cursor will always be closed, and your try block will simply do nothing. – Chat

Answer 6

I think my answer is the best one :

    Cursor cursor = null;

    try {
        cursor = rsd.rawQuery(querySql, null);
        if (cursor.moveToFirst()) {
            do {
                // select your need data from database
            } while (cursor.moveToNext());
        }
    } finally {
        if (cursor != null && !cursor.isClosed()) {
            cursor.close();
            cursor = null;
        }
    }

Proceeding

if this answer is not good, please tell me why you think this model is worse – Proceeding

Answer 7

I think @skylarsutton's is a right answer for the question. However, I want to leave codes for the question (any codes in answers seems to have some flaws). Please consider to use my code.

Cursor c = query(....);
if (c != null) {
   try {        
       //You have to use moveToFirst(). There is no quarantee that a cursor is located at the beginning.
       for(c.moveToFirst();!c.isAfterLast();c.moveToNext()) {  
          // process row...
       }
   } 
   finally {
       c.close();
    }
}

Toxin