You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@commons.apache.org by sebb <se...@gmail.com> on 2010/01/14 01:58:38 UTC
[DBCP] Odd code in SharedPoolDataSource.getPooledConnectionAndInfo()
The method SharedPoolDataSource.getPooledConnectionAndInfo has the
following code:
synchronized (userKeys) {
if (userKeys.containsKey(username)) {
userKeys.remove(username);
}
}
Why not just use userKeys.remove(username)?
What is the purpose of checking the map first?
Also, the keys used for userKeys consist of the username+password, so
this doesn't really make sense anyway - it will only remove an entry
if the password is the empty string.
Note that the synch. block may still be needed, because
LRUMap.remove() - i.e. SequencedHashMap.remove() - updates the
underlying map and some other fields but is not synchronised.
Unfortunately the Javadoc does not say what the class invariants are.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
For additional commands, e-mail: dev-help@commons.apache.org
Re: [DBCP] Odd code in SharedPoolDataSource.getPooledConnectionAndInfo()
Posted by sebb <se...@gmail.com>.
On 14/01/2010, sebb <se...@gmail.com> wrote:
> On 14/01/2010, Phil Steitz <ph...@gmail.com> wrote:
> > Phil Steitz wrote:
> > > sebb wrote:
> > >> The method SharedPoolDataSource.getPooledConnectionAndInfo has the
> > >> following code:
> > >>
> > >> synchronized (userKeys) {
> > >> if (userKeys.containsKey(username)) {
> > >> userKeys.remove(username);
> > >> }
> > >> }
> > >>
> > >> Why not just use userKeys.remove(username)?
> > >
> > > That would work, other than other problem below.
> > >> What is the purpose of checking the map first?
> > >> Also, the keys used for userKeys consist of the username+password, so
> > >> this doesn't really make sense anyway - it will only remove an entry
> > >> if the password is the empty string.
> > >
> > > Yes, it should be username+password, so the code is really doing
> > > nothing now. The concatenation setup, btw, is bogus as there is no
> > > guarantee that you won't have collisions. Probably worth raising a
> > > defect for this.
> >
> >
> > The username+password key was introduced in the fix for DBCP-8. The
> > cleanup code was introduced in the fix for DBCP-245 (at which time
> > "username" by itself was the right key). The fix for DBCP-8 makes
> > the lack of cleanup no longer cause the bug in DBCP-245 to manifest.
> > Should still be fixed, though. I am on the fence on whether or not
> > to reopen DBCP-8. Likelihood of collision is low, but it is possible.
>
>
> I don't really understand the code here, so I don't know why one would
> want to keep a separate list of user keys.
Further investigation shows that it is indeed just being used as a cache.
Also, the key is only ever used by getPooledConnectionAndInfo()
Seems completely pointless caching them.
> It appears to be a cache for the UserPassKey objects, however a
> maximum of 10 are ever saved.
>
> Why not just create the UserPassKey object when you need it?
>
> If different instances of UserPassKey object with same name/password
> are not not equivalent to the borrowObject() method, what happens when
> the LRUMap drops an entry, i.e. a new instance will be generated?
>
borrowObject(key) relies on key.equals() so this is not a problem.
> >
> > Phil
> >
> > >
> > > Good catch, I will fix the cleanup bug.
> > >> Note that the synch. block may still be needed, because
> > >> LRUMap.remove() - i.e. SequencedHashMap.remove() - updates the
> > >> underlying map and some other fields but is not synchronised.
> > >> Unfortunately the Javadoc does not say what the class invariants are.
> > >
> > > Yes the synch is needed.
> > >
> > > Phil
> > >
> > >
> > >> ---------------------------------------------------------------------
> > >> To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
> > >> For additional commands, e-mail: dev-help@commons.apache.org
> > >>
> > >
> >
> >
> > ---------------------------------------------------------------------
> > To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
> > For additional commands, e-mail: dev-help@commons.apache.org
> >
> >
>
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
For additional commands, e-mail: dev-help@commons.apache.org
Re: [DBCP] Odd code in SharedPoolDataSource.getPooledConnectionAndInfo()
Posted by sebb <se...@gmail.com>.
On 14/01/2010, Phil Steitz <ph...@gmail.com> wrote:
> Phil Steitz wrote:
> > sebb wrote:
> >> The method SharedPoolDataSource.getPooledConnectionAndInfo has the
> >> following code:
> >>
> >> synchronized (userKeys) {
> >> if (userKeys.containsKey(username)) {
> >> userKeys.remove(username);
> >> }
> >> }
> >>
> >> Why not just use userKeys.remove(username)?
> >
> > That would work, other than other problem below.
> >> What is the purpose of checking the map first?
> >> Also, the keys used for userKeys consist of the username+password, so
> >> this doesn't really make sense anyway - it will only remove an entry
> >> if the password is the empty string.
> >
> > Yes, it should be username+password, so the code is really doing
> > nothing now. The concatenation setup, btw, is bogus as there is no
> > guarantee that you won't have collisions. Probably worth raising a
> > defect for this.
>
>
> The username+password key was introduced in the fix for DBCP-8. The
> cleanup code was introduced in the fix for DBCP-245 (at which time
> "username" by itself was the right key). The fix for DBCP-8 makes
> the lack of cleanup no longer cause the bug in DBCP-245 to manifest.
> Should still be fixed, though. I am on the fence on whether or not
> to reopen DBCP-8. Likelihood of collision is low, but it is possible.
I don't really understand the code here, so I don't know why one would
want to keep a separate list of user keys.
It appears to be a cache for the UserPassKey objects, however a
maximum of 10 are ever saved.
Why not just create the UserPassKey object when you need it?
If different instances of UserPassKey object with same name/password
are not not equivalent to the borrowObject() method, what happens when
the LRUMap drops an entry, i.e. a new instance will be generated?
>
> Phil
>
> >
> > Good catch, I will fix the cleanup bug.
> >> Note that the synch. block may still be needed, because
> >> LRUMap.remove() - i.e. SequencedHashMap.remove() - updates the
> >> underlying map and some other fields but is not synchronised.
> >> Unfortunately the Javadoc does not say what the class invariants are.
> >
> > Yes the synch is needed.
> >
> > Phil
> >
> >
> >> ---------------------------------------------------------------------
> >> To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
> >> For additional commands, e-mail: dev-help@commons.apache.org
> >>
> >
>
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
> For additional commands, e-mail: dev-help@commons.apache.org
>
>
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
For additional commands, e-mail: dev-help@commons.apache.org
Re: [DBCP] Odd code in SharedPoolDataSource.getPooledConnectionAndInfo()
Posted by Phil Steitz <ph...@gmail.com>.
Phil Steitz wrote:
> sebb wrote:
>> The method SharedPoolDataSource.getPooledConnectionAndInfo has the
>> following code:
>>
>> synchronized (userKeys) {
>> if (userKeys.containsKey(username)) {
>> userKeys.remove(username);
>> }
>> }
>>
>> Why not just use userKeys.remove(username)?
>
> That would work, other than other problem below.
>> What is the purpose of checking the map first?
>> Also, the keys used for userKeys consist of the username+password, so
>> this doesn't really make sense anyway - it will only remove an entry
>> if the password is the empty string.
>
> Yes, it should be username+password, so the code is really doing
> nothing now. The concatenation setup, btw, is bogus as there is no
> guarantee that you won't have collisions. Probably worth raising a
> defect for this.
The username+password key was introduced in the fix for DBCP-8. The
cleanup code was introduced in the fix for DBCP-245 (at which time
"username" by itself was the right key). The fix for DBCP-8 makes
the lack of cleanup no longer cause the bug in DBCP-245 to manifest.
Should still be fixed, though. I am on the fence on whether or not
to reopen DBCP-8. Likelihood of collision is low, but it is possible.
Phil
>
> Good catch, I will fix the cleanup bug.
>> Note that the synch. block may still be needed, because
>> LRUMap.remove() - i.e. SequencedHashMap.remove() - updates the
>> underlying map and some other fields but is not synchronised.
>> Unfortunately the Javadoc does not say what the class invariants are.
>
> Yes the synch is needed.
>
> Phil
>
>
>> ---------------------------------------------------------------------
>> To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
>> For additional commands, e-mail: dev-help@commons.apache.org
>>
>
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
For additional commands, e-mail: dev-help@commons.apache.org
Re: [DBCP] Odd code in SharedPoolDataSource.getPooledConnectionAndInfo()
Posted by Phil Steitz <ph...@gmail.com>.
sebb wrote:
> The method SharedPoolDataSource.getPooledConnectionAndInfo has the
> following code:
>
> synchronized (userKeys) {
> if (userKeys.containsKey(username)) {
> userKeys.remove(username);
> }
> }
>
> Why not just use userKeys.remove(username)?
That would work, other than other problem below.
> What is the purpose of checking the map first?
> Also, the keys used for userKeys consist of the username+password, so
> this doesn't really make sense anyway - it will only remove an entry
> if the password is the empty string.
Yes, it should be username+password, so the code is really doing
nothing now. The concatenation setup, btw, is bogus as there is no
guarantee that you won't have collisions. Probably worth raising a
defect for this.
Good catch, I will fix the cleanup bug.
>
> Note that the synch. block may still be needed, because
> LRUMap.remove() - i.e. SequencedHashMap.remove() - updates the
> underlying map and some other fields but is not synchronised.
> Unfortunately the Javadoc does not say what the class invariants are.
Yes the synch is needed.
Phil
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
> For additional commands, e-mail: dev-help@commons.apache.org
>
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
For additional commands, e-mail: dev-help@commons.apache.org