tomcat-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From bugzi...@apache.org
Subject DO NOT REPLY [Bug 46209] Findbugs report on Tomcat-Pool
Date Fri, 14 Nov 2008 17:03:47 GMT
https://issues.apache.org/bugzilla/show_bug.cgi?id=46209


Sebb <sebb@apache.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|RESOLVED                    |REOPENED
         Resolution|WONTFIX                     |




--- Comment #6 from Sebb <sebb@apache.org>  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: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org


Mime
View raw message