On 14/01/2010, Phil Steitz <phil.ste...@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-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
>
>
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscr...@commons.apache.org
For additional commands, e-mail: dev-h...@commons.apache.org
