https://issues.apache.org/bugzilla/show_bug.cgi?id=46209
Sebb <[EMAIL PROTECTED]> changed:
What |Removed |Added
----------------------------------------------------------------------------
Status|RESOLVED |REOPENED
Resolution|WONTFIX |
--- Comment #6 from Sebb <[EMAIL PROTECTED]> 2008-11-14 09:03:45 PST ---
(In reply to comment #5)
> >H C IL: There is an apparent infinite recursive loop in
> >org.apache.tomcat.jdbc.pool.DataSourceProxy.setMinIdle(int)
> Fixed before the report was filed.
OK.
> >H V MS: org.apache.tomcat.jdbc.pool.ConnectionPool.log isn't final but
> >should be
> Not an issue
But why not fix it to avoid possible later problems?
If the variable is ever changed from a different thread then there will be
potentially two versions active.
All variables should be locked down as far as possible, i.e. private and final.
This considerably reduces the chances of accidental modifications, and makes
the code much easier to test, because there are fewer states for the code to
occupy.
It's very difficult to reduce the visibility of items once code has been
released. However should it prove necessary, visibility can be increased later
- if it really proves necessary.
> >H V MS: org.apache.tomcat.jdbc.pool.DataSourceFactory.log isn't final but
> >should be
> Not an issue
>
> >H V MS:
> >org.apache.tomcat.jdbc.pool.DataSourceFactory$DataSourceHandler.methods
> >isn't final but should be
> Not an issue
>
> >H V MS: org.apache.tomcat.jdbc.pool.DataSourceProxy.log isn't final but
> >should be
> Not an issue
>
> >H V MS: org.apache.tomcat.jdbc.pool.Driver.pooltable isn't final but should
> >be
> Not an issue
>
> >H V MS: org.apache.tomcat.jdbc.pool.PooledConnection.log isn't final but
> >should be
> Not an issue
>
> >M B ES: Comparison of String objects using == or != in
> >org.apache.tomcat.jdbc.pool.interceptor.ConnectionState.invoke(Object,
> >Method, Object[])
> Classes, methods, and fields, whether referenced from Java Virtual Machine
> instructions or from other constant pool entries, are named using the constant
> pool.Relies on the method name being stored in the constant pool, it'd be
> interesting to see if that actually wasn't the case.
> The spec says "Classes, methods, and fields, whether referenced from Java
> Virtual Machine instructions or from other constant pool entries, are named
> using the constant pool."
Nevertheless, why not do it safely?
>
> >M B ES: Comparison of String objects using == or != in
> >org.apache.tomcat.jdbc.pool.interceptor.SlowQueryReport.process(String[],
> >Method, boolean)
> Classes, methods, and fields, whether referenced from Java Virtual Machine
> instructions or from other constant pool entries, are named using the constant
> pool.Relies on the method name being stored in the constant pool, it'd be
> interesting to see if that actually wasn't the case.
> The spec says "Classes, methods, and fields, whether referenced from Java
> Virtual Machine instructions or from other constant pool entries, are named
> using the constant pool."
>
Ditto.
> >M B It: org.apache.tomcat.jdbc.pool.FairBlockingQueue$FairIterator.next()
> >can't throw NoSuchElementException
> Not an issue, although I dont even understand the bug reprot
It's probably because next() implements Iterator.next() which throws this.
However, it can throw ArrayOutOfBounds which should probably be converted to
NoSuchElement.
> >M B Nm: The class name org.apache.tomcat.jdbc.pool.DataSource shadows the
> >simple name of implemented interface javax.sql.DataSource
> Not an issue
It's not a coding bug but it is a maintenance bug waiting to happen.
Now is the time to fix this - before the first release.
> >M B Nm: The class name org.apache.tomcat.jdbc.pool.Driver shadows the simple
> >name of implemented interface java.sql.Driver
> Not an issue
Ditto.
> >M D DLS: Dead store to exec in
> >org.apache.tomcat.jdbc.pool.PooledConnection.validate(int, String)
> Not an issue
Not a bug, but why store the response if you are not using it?
> >M D Dm: Call to unsupported method
> >org.apache.tomcat.jdbc.pool.FairBlockingQueue.drainTo(Collection, int) in
> >org.apache.tomcat.jdbc.pool.FairBlockingQueue.drainTo(Collection)
> Not an issue
Agreed.
> >M D NP: Load of known null value in
> >org.apache.tomcat.jdbc.pool.ConnectionPool.borrowConnection()
> Not an issue
True, but why pass con to createConnection() when that does not use it?
Would it not be clearer to drop the con parameter?
> >M D REC: Exception is caught when Exception is not thrown in
> >org.apache.tomcat.jdbc.pool.ConnectionPool.getConnection()
> One would hope so :) not an issue
Agreed, but surely the original exception should be passed to the SQLException?
> >M M UL: org.apache.tomcat.jdbc.pool.FairBlockingQueue.poll(long, TimeUnit)
> >does not release lock on all exception paths
> >M M UL: org.apache.tomcat.jdbc.pool.FairBlockingQueue.poll(long, TimeUnit)
> >does not release lock on all exception paths
> I'd have to challenge that
Looks like the analyser was confused by the error flag, but it's not easy to
follow.
> >M P SIC: Should org.apache.tomcat.jdbc.pool.ConnectionPool$PoolCleaner be a
> >_static_ inner class?
> Not an issue
It's just a warning.
> >M V EI2: new
> >org.apache.tomcat.jdbc.pool.interceptor.SlowQueryReport$StatementProxy(SlowQueryReport,
> > Object, Object[]) may expose internal representation by storing an
> >externally mutable object into SlowQueryReport$StatementProxy.args
> not an issue
This is a generic error; probably does not matter here, but for some ccode it
could be extremely important.
> >M V MS: org.apache.tomcat.jdbc.pool.DataSourceFactory.ALL_PROPERTIES should
> >be package protected
> Not an issue
But why not fix it?
> >M X OBL: Method org.apache.tomcat.jdbc.pool.PooledConnection.validate(int,
> >String) may fail to clean up stream or resource of type java.sql.Statement
> Not an issue
On the contrary, it is a bug - if the execute() method throws an Exception, the
statement won't be closed.
> >There are quite a few cases of class variables that are not final, even
> >though
> >they are set in the constructor and not actually changed thereafter.
> Not an issue
>
> >Wherever possible, variables should be made final to ensure thread-safety.
> If there is a real bug, we would fix it.
If the classes are intended to be used by multiple threads, then there is an
issue. It's rare, but it happens. The problem is caused by the memory model
which allows values to be cached. Note that this was found to be a problem with
java.lang.String, whose fields were made final in Java 1.5.
> >There are some variables that are protected, even though there is already an
> >access method for them.
> Not an issue
Why allow uncontrolled access?
> >For example, DataSourceProxy:driver
> >In this case, the variable should definitely be private, as access needs to
> >be
> >synchronized.
> Dont see an issue here either, however, I would switch
>
> >PooledConnection:counter is volatile, presumably instead of synchronising it.
> >However, volatile does not protect against the read-modify-write involved in
> >processing the counter++ statement.
> Will switch it to an atomic int, although it wont make a difference since the
> value is not used anywhere important in the code
>
> >Same applies to PoolProperties:poolCounter
> Made to atomic integer
>
> >Also, both of these are protected. I think they should be final.
> not an issue
>
> >It would be useful to use Javadoc to state the class thread-safety or
> >otherwise, and to add details of how fields are synchronised, e.g. using the
> >@GuardedBy annotation.
> yes, that would be nice
>
> >It's confusing to have aliases such as:
> >final ReentrantLock lock = FairBlockingQueue.this.lock;
> >when the code works just the same without.
> code practise, to ensure the global lock reference is not set to null. will
> not
> happen here, but but so goes for 90% of this report.
>
> >Some source files still have the following copyright:
> >Copyright: Copyright (c) 2008 Filip Hanik
> will clean up
>
--
Configure bugmail: https://issues.apache.org/bugzilla/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.
---------------------------------------------------------------------
To unsubscribe, e-mail: [EMAIL PROTECTED]
For additional commands, e-mail: [EMAIL PROTECTED]
