synchronized on local variable, better than synchronized methods?

Question

I wrote a wrapper around database queries and need to access it from different threads. Therefore my application creates exactly one instance of that helper class and returns it through a getter.

DbConnection dbc = app.getDatabaseConnection();
synchronized (dbc) {
  dbc.doSomething();
}

Is this code safe? As stated here it should work although synchronizing on a local variable. Is this correct as long as the object instance is guaranteed to be the same?

Is it a better approach to make all affected instance methods of DbConnection synchronized?

If possible you should make `dbc` be `private` and more importantly `final`. — Gray, Nov 01 '13 at 17:06

score 3 · Accepted Answer · answered Nov 01 '13 at 17:07

3

That's not good design.

Instead of making your DBConnection class inherently thread-safe by making its methods/blocks synchronized when necessary, you force all the clients of this class to explicitely synchronize each time it's needed. So instead of encapsulating the thread-safety in a single, well-dentified class, you distribute this responsibility among all the clients of the class, making the whole thing extremely fragile, and a potential bug extremely hard to find out.

That said, using a single database connection from multiple threads is, in itself, a bad idea.

answered Nov 01 '13 at 17:07

JB Nizet

678,734
91
1,224
1,255

It is a locking database connection, there can only be one connection. I known that synchronization comes with disadvantages here, but as the application transmits small amounts of data, that should not be a problem. – just lerning Nov 01 '13 at 17:10
So, assuming the underlying DB connection is thread-safe (which is already a wild guess), let's say that thread A starts a transaction, then unrelated thread B also tries to start one. How do you intend to make that work? Why not use a pool of connections, even reduced to 1 connection if really needed. That would at least ensure that two threads don't use the same connection at the same time. – JB Nizet Nov 01 '13 at 17:16
There is really little traffic to the database. If all transactions are performed by the adapter (DbConnection) which provides synchronized methods, shouldn't it work? I am sorry if this is a dumb question but I did not work much with databases and thread-safety in the past. (And I agree, the class name is misleading. Going to change that within some commits.) – just lerning Nov 01 '13 at 17:22
It will work if the whole transaction is done inside a single synchronized method. If you just synchronize getDatabaseConnection(), then two threads won't be able to get the connection at the same time. But once they both successfully get it (sequentially), they can still use it concurrently. – JB Nizet Nov 01 '13 at 17:28
It should work if I use `synchronized void writeSomething` and `synchronized ... readSomething` (assuming both are instance methods of DbConnection), shouldn't it? – just lerning Nov 01 '13 at 17:40
It all depends on what these two methods do. A transaction usually has much more to do than simply write something to the database. Use a connection pool. That's what they're for: accessing the database from multiple threads in a safe way. – JB Nizet Nov 01 '13 at 17:45

score 1 · Answer 2 · answered Nov 01 '13 at 17:07

If all your instance methods of DbConnection need to be synchronized, then make all the methods synchronized. Don't look at the amount of code you write, look only at correctness. If you synchronize each method, you don't have a chance of going back years from now and calling getDatabaseConnection then forgetting to synchronize.

synchronized on local variable, better than synchronized methods?

2 Answers2