Re: Bug in TypeInfoCache causes getObject to fail

Hello,

i have found a race condition in org.postgresql.jdbc2.TypeInfoCache. A
Map is created as a static member, but reinitialized for every new
instance of TypeInfoCache. That causes a race condition in
ResultSet.getObject(...), which then returns PGobject instead of the
correct type. Probably causes other problems as well. I have attached a
test case. The test case works best on smp machines (tested on a 4 way),
where it fails almost every time.

Also the maps in TypeInfoCache are created as synchronizedMaps, which is
unneccessary, slow and a potential source for further side effects. I
have removed that as well, and attached a fixed version of TypeInfoCache
from build 404. I leave the application of the fixes to the current and
other versions as an exercise to the committer ;-)

Have a nice day...

Till

On Thu, 9 Feb 2006, till toenges wrote:

> i have found a race condition in org.postgresql.jdbc2.TypeInfoCache. A Map is
> created as a static member, but reinitialized for every new instance of
> TypeInfoCache. That causes a race condition in ResultSet.getObject(...),
> which then returns PGobject instead of the correct type. Probably causes
> other problems as well. I have attached a test case. The test case works best
> on smp machines (tested on a 4 way), where it fails almost every time.

Nice work. Applied to 8.1 and HEAD.

> Also the maps in TypeInfoCache are created as synchronizedMaps, which is
> unneccessary, slow and a potential source for further side effects. I have
> removed that as well, and attached a fixed version of TypeInfoCache from
> build 404. I leave the application of the fixes to the current and other
> versions as an exercise to the committer ;-)

Actually it is necessary for the maps to be synchronized because at any
time a client may call PGConnection.addDataType which will modify the
maps.

Kris Jurka

Kris Jurka wrote:
> Nice work. Applied to 8.1 and HEAD.

Thank you!

> Actually it is necessary for the maps to be synchronized because at any
> time a client may call PGConnection.addDataType which will modify the maps.

Ah, i forgot that the driver needs to be thread safe. I looked at it
again, and made a few changes to improve this in other fringe cases.

Mostly, i checked where and how the maps were used. I also noticed that
there are two PreparedStatements, which might be not thread safe (don't
know enough about the internals).

The only map that can be accessed from outside the class is
_pgNameToSQLType, via an iterator returned from
getPGTypeNamesWithSQLType(). That must obviously be thread safe, more on
that below. The other maps are not used outside the class, and can be
protected by synchronizing the accessing methods.

Also, the other methods often rely on a consistent relation between two
maps. A simple synchronizedMap ensures not that both maps are in a
consistent state relative to each other. Therefore any method accessing
any of these maps should be synchronized as a whole.

You wrote in the changelog that _pgNameToSQLType is actually static.
That can be done in a static initializer, and the member can be static
(and final) again. To protect it against any changes, and therefore make
it thread safe, an unmodifiableMap seems enough.

The methods using the PreparedStatements must be synchronized, because
the statements are only created when neccessary, so they can not be used
to synchronize on. They also fall under the category of methods that use
more than one map at once, see above.

I made another patch to the current branch, which i attached.

Hope some of this makes sense ;-)

Till