On 14/01/2010, sebb <seb...@gmail.com> wrote:
> 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.
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-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
