MySQL Bugs: #24721: NPE in Statement.cancel() (again)

Bug #24721	NPE in Statement.cancel() (again)
Submitted:	30 Nov 2006 10:39	Modified:	27 Feb 2007 13:52
Reporter:	Damián Arregui	Email Updates:
Status:	Closed	Impact on me:	None
Category:	Connector / J	Severity:	S2 (Serious)
Version:	5.0.4	OS:
Assigned to:		CPU Architecture:	Any

Description:
NPE thrown in Statement.cancel() line 346:

cancelStmt.execute("KILL QUERY " + this.connection.getIO().getThreadId());

This results from a race condition when calling Statement.close() and Statement.cancel() from two different threads.

Statement.close() sets this.connection to null.

Statement.close() checks that this.connection is not null before using it but these operations are not performed in an atomic way: the value of this.connection may be changed after the test by a concurrent thread.

Hence if a call to Statement.close() slips in just before this.connection is used in Statement.cancel() we will get an NPE. 

How to repeat:
Use a Java debugger. Create a statement and have one thread calling Statement.close() and another one calling Statement.cancel(). Proper step-by-step execution will allow to reproduce the above-described situation.

Suggested fix:
Execution of Statement.close() and Statement.cancel() should be mutually exclusive.

Hi Damián and thanks for your report. I will set it to "Verified" although I don't think it's realy a bug. And here's why:
	/**
	 * Cancel can be used by one thread to cancel a statement that is being
	 * executed by another thread. However this driver is synchronous, so this
	 * really has no meaning - we define it as a no-op (i.e. you can't cancel,
	 * but there is no error if you try.)
	 * 
	 * @exception SQLException
	 *                only because thats the spec.
	 */
	public void cancel() throws SQLException {
		// No-op
	}

Test case result:
.Loading JDBC driver 'com.mysql.jdbc.Driver'
Done.

Done.

Threads Joining.
*Close* thread started
*Cancel* thread started
Connected to 5.0.27-log
*Close* thread, about to do stmt.execute
*Cancel* thread, about to do stmt.cancel()
*Close* thread, stmt.execute done
*Close* thread, about to do stmt.close
*Close* thread, stmt.close done
*Close* thread terminated
Exception in thread "Cancel thread" java.lang.NullPointerException
	at com.mysql.jdbc.Statement.cancel(Statement.java:347)
	at testsuite.simple.TestBug24721$CancelThread.run(TestBug24721.java:56)
	at java.lang.Thread.run(Unknown Source)

Time: 1,875

OK (1 test)

Test case

Attachment: TestBug24721.java (text/x-java), 3.88 KiB.

Hi Tonci,

I'm not sure to understand your explanation about why this is not a bug. I got a different javadoc for Statement.cancel() in the source file:

/**
 * Cancels this Statement object if both the DBMS and driver support
 * aborting an SQL statement. This method can be used by one thread to
 * cancel a statement that is being executed by another thread.
 */
public void cancel() throws SQLException { ...

Sorry Damián, my Eclipse workspace is totally broken... From branch 5.0 test it fetched branch 3.1 code thus the code I pasted...

Hi Damián. Can you please test with latest sources. When I hit 5.0 code branch at last, my tets case works as expected.

.Loading JDBC driver 'com.mysql.jdbc.Driver'
Done.

Done.

*Close* thread started
Threads Joining.
*Cancel* thread started
Connected to 5.0.27-log
*Close* thread, about to do stmt.execute
*Close* thread, stmt.execute done
*Close* thread, about to do stmt.close
*Close* thread, stmt.close done
*Close* thread terminated
*Cancel* thread, about to do stmt.cancel()
*Cancel* thread, stmt.cancel() done
*Cancel* thread terminated

Time: 1,812

OK (1 test)

Damián, can you try my test case on your connector version and see if it fails? If so, upgrading c/J should fix the problem.

I do not think your test case allows to reliably reproduce the problem. The only way to do it is using a step-by-step debugger and forcing the proper interleaving of the Statement.cancel() and Statement.close() methods.

In the last trace you posted executions of these two methods are not interleaved, which explains why you don't get the NPE.

New "interleaved" test case

Attachment: TestBug24721.java (text/x-java), 2.92 KiB.

"Interleaved" test case output

Attachment: 24721-test2.zip (application/zip, text), 19.08 KiB.

Hi Damián. I am really trying to get this properly verified but I can't! Please examine my 2nd test output and tell me where to "interleave" in a different way to get this happen... As for debugger, I can't take that as relevant since you need kernel debug session to be absolutely sure what's happening...

To reproduce:

1) Set a breakpoint at line 346 in Statement.cancel()
2) Set a breakpoint at line 2006 in Statment.realClose()
3) Run your program in debug mode, one thread should be paused at each breakpoint
4) Resume execution of thread suspended in Statment.realClose()
5) Resume execution of thread suspended in Statment.cancel()
=> You get an NPE!

This is a legal scheduling of threads. There is no guarantee that this would not happen. How often you will actually see this NPE depends on your application, OS, JVM, system load, etc.

The bottom line is that with the current code it is not safe to call Statement.close() and Statement.cancel() on the same Statement from two different threads.

Please see the following changelog comment from 5.0.0:

"Removed work done for BUG#14652, and instead loosened synchronization
      to solve a number of deadlock issues in BUG#18719, BUG#18367, BUG#17709
      and BUG#15067. New strategy basically makes Connection instances threadsafe
      and thus shareable across threads, and anything else threadsafe, but not 
      necessarily shareable across threads due to JDBC API interactions that 
      can cause non-obvious behavior and/or deadlock scenarios to occur since
      the API is not designed to be used from multiple threads at once. 
      
      Therefore, unless external synchronization is provided, clients should
      not allow multiple threads to share a given statement or result set. Examples
      of issues with the API itself not being multi-thread suitable include, 
      but are not limited to race conditions between modifiers and execution and 
      retrieval methods on statements and result sets that are not synchronizable 
      such as ResultSet.get*() and traversal methods, or Statement.execute*() closing 
      result sets without effectively making the driver itself serializable across the 
      board.
      
      These changes should not have any effect on "normal" J(2)EE use cases
      where only one thread ever uses a connection instance and the objects created by
      it."

Also, note that we're not the only vendor to come to this conclusion. 

We can probably fix this one case, but synchronization isn't the solution, due to the deadlocks that can happen.

Hi Mark,

In the sources I have for version 5.0.4, javadoc for Statement.cancel() says:

"This method can be used by one thread to cancel a statement that is being executed by another thread."

Which makes sense to me. More than any other part of the JDBC API, the particular case of Statement.cancel() is calling for thread-safety. The use case we had here is as follows:

- one thread tries to perform standard Statement execution and then to close the Statement
- however, this Statement execution blocks (for whatever reason)
- a separate thread detects that Statement execution has been blocking for too long and tries to cancel it
- simultaneously the Statement execution returns and hence we try to close the Statement

That's how we get concurrent executions of Statement.cancel() and Statement.close() which lead to an NPE.

Fixed for 5.0.5, see nightly snapshot at http://downloads.mysql.com/snapshot.php#connector-j

A note has been added to the 5.0.5 changelog.