Re: binary patch problems

Hi Mikko,

My suggestion would be to fix it.

Dave Cramer

dave.cramer(at)credativ(dot)ca
http://www.credativ.ca

On Fri, Sep 16, 2011 at 7:14 PM, Oliver Jowett <oliver(at)opencloud(dot)com> wrote:
> On 17 September 2011 01:20, Mikko Tiihonen
> <mikko(dot)tiihonen(at)nitorcreations(dot)com> wrote:
>
>> If I debugged it correctly it looks like the query statement is closed by
>> finalizer during the test which in turn closes the relevant portals.
>> If I add stmt.close() to the end of the test there are no failures as it
>> keeps the GC from killing of the statement during the test.
>>
>> But why I only seem to get it when running with binary transfer patches I do
>> not know. So there must be something in the patches that I cannot spot.
>
> (I am halfway through my first coffee of the day. YMMV)
>
> The test code holds a reference to the returned ResultSet, but doesn't
> keep a reference to the Statement itself after calling executeQuery().
> Normally this isn't a problem because there is a strong ref from a
> ResultSet to its owning Statement, so while you are using a particular
> ResultSet the creating statement remains reachable.
>
> The problem seems to be introduced in the patch to executeQuery if
> (ForceBinaryTransfers). This delegates to a new PreparedStatement
> ("inner PreparedStatement") and splices its resultset ("inner
> resultset") back into the surrounding Statement ("outer Statement")
> and returns that.
>
> However the inner ResultSet has the wrong parent Statement for that
> context (ResultSet.getStatement() will return the "inner
> PreparedStatement" not the "outer Statement") - which is a separate
> problem, but also causes the GC problems as well, as now there's no
> strong ref from the returned ResultSet to the Statement that the
> client created it via.
>
> So in the test code we end up with:
>  (a) a strong ref to the inner ResultSet, directly; and
>  (b) a strong ref to the inner PreparedStatement, via the statement
> reference of the inner ResultSet.
>
> .. but no strong ref to the outer Statement. So the outer Statement
> becomes garbage right after the call to executeQuery(), and if you are
> unlucky with GC timing, it can be finalized and closed under you,
> which then closes the inner ResultSet.
>
> Adding a call to stmt.close() means that the outer Statement still has
> a strong reference directly from test code up until that point, so it
> doesn't get finalized early.
>
> At a glance, executeQuery() is also broken for queries that return
> multiple resultsets if ForceBinaryTransfers is enabled - you only get
> the first resultset back.
>
> Oliver
>