Re: [VOTE] Release of DbUtils 1.2 RC2
I'd suggest marking it volatile or making it an immutable property. the overhead incurred from enforcing thread safety i think is a bit much for the specific purpose of the QueryRunner -- in all the instances that you mentioned -- it's the datasource that should be dispatched to threads not the queryRunner anyways. On Sun, Mar 15, 2009 at 2:52 PM, sebb seb...@gmail.com wrote: On 15/03/2009, Dan Fabulich d...@fabulich.com wrote: sebb wrote: OK, I'd not noticed that the class was usable without the DataSource. Of course the alternative is to document the class as thread-unsafe... I would guess that the reason we've never seen a bug filed on this issue is that nobody uses setDataSource after the class is created. For these users, QueryRunner is thread-safe. I think just formalizing that state is best. If you mean nobody uses setDataSource at all, then I agree that cannot affect thread-safety. However, if anyone uses setDataSource (which has to be after creation) and passes the instance to another thread, then the receiving thread may not see the updated value for the ds variable, i.e. it would not be thread-safe. I would not attempt to synchronize this class, just leave it unsafe and let users synchronize. We should document more explicitly that (unlike some other classes in DbUtils) it's unsafe. I'm not sure that the class can be made thread-safe externally. It's easy enough to override the setters with synchronized versions, but the getters need to be synchronized as well to ensure that the data is published correctly. However the class stores the unsynchronized getters in the Map. So it would be necessary to override invoke() as well. If this is done, then the whole class has been overridden - one might as well say it has been rewritten. I didn't mean that users would synchronize externally by extending/overriding, but just by synchronizing access to an instance member, or just not sharing them across threads. *shrug* For a mutable instance field to be thread-safe, both writes and reads need to be synchronized (or volatile). It's not enough to synch. just the writes, and readers and writers must all use the same lock. -Dan - To unsubscribe, e-mail: dev-unsubscr...@commons.apache.org For additional commands, e-mail: dev-h...@commons.apache.org - To unsubscribe, e-mail: dev-unsubscr...@commons.apache.org For additional commands, e-mail: dev-h...@commons.apache.org
Re: [VOTE] Release of DbUtils 1.2 RC2
sebb wrote: OK, I'd not noticed that the class was usable without the DataSource. Of course the alternative is to document the class as thread-unsafe... I would guess that the reason we've never seen a bug filed on this issue is that nobody uses setDataSource after the class is created. For these users, QueryRunner is thread-safe. I think just formalizing that state is best. I would not attempt to synchronize this class, just leave it unsafe and let users synchronize. We should document more explicitly that (unlike some other classes in DbUtils) it's unsafe. I'm not sure that the class can be made thread-safe externally. It's easy enough to override the setters with synchronized versions, but the getters need to be synchronized as well to ensure that the data is published correctly. However the class stores the unsynchronized getters in the Map. So it would be necessary to override invoke() as well. If this is done, then the whole class has been overridden - one might as well say it has been rewritten. I didn't mean that users would synchronize externally by extending/overriding, but just by synchronizing access to an instance member, or just not sharing them across threads. *shrug* -Dan - To unsubscribe, e-mail: dev-unsubscr...@commons.apache.org For additional commands, e-mail: dev-h...@commons.apache.org
Re: [VOTE] Release of DbUtils 1.2 RC2
On 15/03/2009, Dan Fabulich d...@fabulich.com wrote: sebb wrote: OK, I'd not noticed that the class was usable without the DataSource. Of course the alternative is to document the class as thread-unsafe... I would guess that the reason we've never seen a bug filed on this issue is that nobody uses setDataSource after the class is created. For these users, QueryRunner is thread-safe. I think just formalizing that state is best. If you mean nobody uses setDataSource at all, then I agree that cannot affect thread-safety. However, if anyone uses setDataSource (which has to be after creation) and passes the instance to another thread, then the receiving thread may not see the updated value for the ds variable, i.e. it would not be thread-safe. I would not attempt to synchronize this class, just leave it unsafe and let users synchronize. We should document more explicitly that (unlike some other classes in DbUtils) it's unsafe. I'm not sure that the class can be made thread-safe externally. It's easy enough to override the setters with synchronized versions, but the getters need to be synchronized as well to ensure that the data is published correctly. However the class stores the unsynchronized getters in the Map. So it would be necessary to override invoke() as well. If this is done, then the whole class has been overridden - one might as well say it has been rewritten. I didn't mean that users would synchronize externally by extending/overriding, but just by synchronizing access to an instance member, or just not sharing them across threads. *shrug* For a mutable instance field to be thread-safe, both writes and reads need to be synchronized (or volatile). It's not enough to synch. just the writes, and readers and writers must all use the same lock. -Dan - To unsubscribe, e-mail: dev-unsubscr...@commons.apache.org For additional commands, e-mail: dev-h...@commons.apache.org - To unsubscribe, e-mail: dev-unsubscr...@commons.apache.org For additional commands, e-mail: dev-h...@commons.apache.org
Re: [VOTE] Release of DbUtils 1.2 RC2
+1, all tests pass now also on IBM JDK 6. Dan Fabulich wrote: My second attempt at releasing a commons project; please be gentle. :-) RC2 includes sebb's patches that make numerous instance variables immutable. NOTE: No one has yet explicitly said on-list that they have tested DbUtils 1.2 RC1 or RC2 with a real database. We should not release it until somebody tries it out with a real live Oracle database, as described below. Compatibility warnings: * We upgraded the JVM dependency from JDK 1.3 to JDK 1.4 (DBUTILS-31) * Users who may have extended BeanListHandler.handleRow will find that this method no longer exists (is no longer called) in DbUtils 1.2 (DBUTILS-37) * Users who may have extended KeyedHandler will find that its protected members are now final (to guarantee thread safety). (DBUTILS-51) PLEASE TEST THIS RELEASE WITH A REAL DATABASE! Although this project has reasonable unit tests, it has no integration tests with any actual databases; it is quite possible that the fix for DBUTILS-31 has broken something on Oracle, MS SQL Server, Derby, or your favorite database. To verify DBUTILS-31, use QueryRunner to put a null value in a field, e.g. with QueryRunner.update. Ideally it would be good to verify putting nulls in fields of various types: char, varchar, int, boolean, date, etc. -- Tag: https://svn.apache.org/repos/asf/commons/proper/dbutils/tags/DBUTILS_1_2 Site: http://people.apache.org/builds/commons/dbutils/1.2/RC2/site/index.html Binaries: http://people.apache.org/builds/commons/dbutils/1.2/RC2/staged/commons-dbutils/commons-dbutils/1.2/ [ ] +1 release it [ ] +0 go ahead I don't care [ ] -1 no, do not release it because - To unsubscribe, e-mail: dev-unsubscr...@commons.apache.org For additional commands, e-mail: dev-h...@commons.apache.org
Re: [VOTE] Release of DbUtils 1.2 RC2
Sorry, my last e-mail mentioned that QueryRunner was not thread-safe, but I did not provide a patch. The DataSource variable is protected. To allow multi-threaded access, either the variable has to be made volatile, or it has to be made private (i.e. an API change) and the accessors need to be synchronised, and the class has to use the accessors. Or you could change the API further and insist that the DataSource is provided at construction time; the variable could be then made final. Two of the constructors would need to be removed as well. There's another potential problem with QueryRunner, in that the constructor calls an overrideable public method. This could cause problems if the class is extended, as the subclass may not be fully constructed when the method is invoked. But an easy fix is to add a private set method used by both the constructor and the public method. I did not check the wrappers previously. SqlNullCheckedResultSet has many variables that cannot be made final, but the Javadoc does not claim it is thread-safe. It could be made thread-safe by synchronizing (or making volatile) all the variables, but this might be too much overhead. Depends whether this class is likely to be used from multiple threads or not. On 11/03/2009, Dan Fabulich d...@fabulich.com wrote: My second attempt at releasing a commons project; please be gentle. :-) RC2 includes sebb's patches that make numerous instance variables immutable. NOTE: No one has yet explicitly said on-list that they have tested DbUtils 1.2 RC1 or RC2 with a real database. We should not release it until somebody tries it out with a real live Oracle database, as described below. Compatibility warnings: * We upgraded the JVM dependency from JDK 1.3 to JDK 1.4 (DBUTILS-31) * Users who may have extended BeanListHandler.handleRow will find that this method no longer exists (is no longer called) in DbUtils 1.2 (DBUTILS-37) * Users who may have extended KeyedHandler will find that its protected members are now final (to guarantee thread safety). (DBUTILS-51) PLEASE TEST THIS RELEASE WITH A REAL DATABASE! Although this project has reasonable unit tests, it has no integration tests with any actual databases; it is quite possible that the fix for DBUTILS-31 has broken something on Oracle, MS SQL Server, Derby, or your favorite database. To verify DBUTILS-31, use QueryRunner to put a null value in a field, e.g. with QueryRunner.update. Ideally it would be good to verify putting nulls in fields of various types: char, varchar, int, boolean, date, etc. -- Tag: https://svn.apache.org/repos/asf/commons/proper/dbutils/tags/DBUTILS_1_2 Site: http://people.apache.org/builds/commons/dbutils/1.2/RC2/site/index.html Binaries: http://people.apache.org/builds/commons/dbutils/1.2/RC2/staged/commons-dbutils/commons-dbutils/1.2/ [ ] +1 release it [ ] +0 go ahead I don't care [ ] -1 no, do not release it because - To unsubscribe, e-mail: dev-unsubscr...@commons.apache.org For additional commands, e-mail: dev-h...@commons.apache.org - To unsubscribe, e-mail: dev-unsubscr...@commons.apache.org For additional commands, e-mail: dev-h...@commons.apache.org
Re: [VOTE] Release of DbUtils 1.2 RC2
sebb wrote: Sorry, my last e-mail mentioned that QueryRunner was not thread-safe, but I did not provide a patch. Dang; I skimmed through other classes looking for unsafe members but overlooked your main point. Or you could change the API further and insist that the DataSource is provided at construction time; the variable could be then made final. This is the right fix. (In fact, in my haste I thought you'd already done it!) We should change the API as necessary to make the class immutable. Two of the constructors would need to be removed as well. No, you could just set the DataSource to null in the constructor; its Connection-less methods wouldn't work until you created a new object, but I think that's fine. SqlNullCheckedResultSet has many variables that cannot be made final, but the Javadoc does not claim it is thread-safe. It could be made thread-safe by synchronizing (or making volatile) all the variables, but this might be too much overhead. Depends whether this class is likely to be used from multiple threads or not. I would not attempt to synchronize this class, just leave it unsafe and let users synchronize. We should document more explicitly that (unlike some other classes in DbUtils) it's unsafe. -Dan - To unsubscribe, e-mail: dev-unsubscr...@commons.apache.org For additional commands, e-mail: dev-h...@commons.apache.org
Re: [VOTE] Release of DbUtils 1.2 RC2
On 11/03/2009, Dan Fabulich d...@fabulich.com wrote: sebb wrote: Sorry, my last e-mail mentioned that QueryRunner was not thread-safe, but I did not provide a patch. Dang; I skimmed through other classes looking for unsafe members but overlooked your main point. Or you could change the API further and insist that the DataSource is provided at construction time; the variable could be then made final. This is the right fix. (In fact, in my haste I thought you'd already done it!) We should change the API as necessary to make the class immutable. Two of the constructors would need to be removed as well. No, you could just set the DataSource to null in the constructor; its Connection-less methods wouldn't work until you created a new object, but I think that's fine. OK, I'd not noticed that the class was usable without the DataSource. Of course the alternative is to document the class as thread-unsafe... SqlNullCheckedResultSet has many variables that cannot be made final, but the Javadoc does not claim it is thread-safe. It could be made thread-safe by synchronizing (or making volatile) all the variables, but this might be too much overhead. Depends whether this class is likely to be used from multiple threads or not. I would not attempt to synchronize this class, just leave it unsafe and let users synchronize. We should document more explicitly that (unlike some other classes in DbUtils) it's unsafe. I'm not sure that the class can be made thread-safe externally. It's easy enough to override the setters with synchronized versions, but the getters need to be synchronized as well to ensure that the data is published correctly. However the class stores the unsynchronized getters in the Map. So it would be necessary to override invoke() as well. If this is done, then the whole class has been overridden - one might as well say it has been rewritten. -Dan - To unsubscribe, e-mail: dev-unsubscr...@commons.apache.org For additional commands, e-mail: dev-h...@commons.apache.org - To unsubscribe, e-mail: dev-unsubscr...@commons.apache.org For additional commands, e-mail: dev-h...@commons.apache.org
