DBConnection finialization bug

I've been getting memory protection violations with the #free of
DBStatements during finalization. I think that I finally have a handle
on what's going on, but I'm looking for guidance from the more
knowledgeable as to how to fix it.

Whether the problem occurs  is predicated on GC and finalization timing
issues, so it's been fun (not!) trying to pin it down. It looks like the
sequence of things is thus:

    - problem DBStatement gets GCed
    - it gets removed from DBConnection statements WeakSet.
    - DBConnection gets #disconnect (either explicitly or as a result of
its finalization)
    - DBConnection invalidateAllStmts does not free problem statement
since it's no longer in the WeakSet
    - does SQLDisconnect, which internally frees all of the associated
statements
    - finalization for problem DBStatement occurs.
    - SQLFreeHandle gets called on the statement that was already freed
by SQLDisconnect - BOOM

The intention obviously is to free all the statements explicitly before
the disconnect, so that the SQLDisconnect won't have any to free
implicitly. With the explicit free, the handle is reset so that the
later finalization doesn't attempt to free the statement again. But with
the implicit free by SQLDisconnect, it's a problem.

I could make DBConnection's statements Set not be weak, but that sure
seems like an ugly way to try and solve it.

Any ideas?
-Bill

-------------------------------------------
Bill Dargel            [hidden email]
Shoshana Technologies
100 West Joy Road, Ann Arbor, MI 48105  USA

Bill,

> I've been getting memory protection violations with the #free of
> DBStatements during finalization. I think that I finally have a handle
> on what's going on, but I'm looking for guidance from the more
> knowledgeable as to how to fix it.

Which OS are you using?  However, I will **NOT** tell you that NT/2k/XP will
fix all such problems; I have seen memory violations associated with ODBC on
2k.

My "solution" has long been to clean up statements as I used them, and the
exact mix required to keep things running has changed over time.  I've added
#sqlCleanup to both DBStatement and DBConnection, as follows:

!DBConnection methodsFor!

sqlCleanup
 "Release resources prior to finalization"

 ^self close! !
!DBConnection categoriesFor: #sqlCleanup!public! !

!DBStatement methodsFor!

sqlCleanup
 "Release resources prior to finalization"

 ^self free! !
!DBStatement categoriesFor: #sqlCleanup!public! !

It's inefficient, but it works.


Interesting.  I know _something_ isn't right, so here's hoping you found it.

Have a good one,

Bill

--
Wilhelm K. Schwab, Ph.D.
[hidden email]

Bill,

Thanks for the response. I'd still much rather figure out a good fix for the
underlying problem, rather than putting in ways to manage and work around it.
More comments below.

Bill Schwab wrote:

> Bill Dargel wrote:

> > I've been getting memory protection violations with the #free of
> > DBStatements during finalization. I think that I finally have a handle
> > on what's going on, but I'm looking for guidance from the more
> > knowledgeable as to how to fix it.
>
> Which OS are you using?  However, I will **NOT** tell you that NT/2k/XP will
> fix all such problems; I have seen memory violations associated with ODBC on
> 2k.

This has been on Win2000 5.0.2195 SP2, with MySQL ODBC 2.50.39 on core ODBC
3.520.6019.

> My "solution" has long been to clean up statements as I used them, and the
> exact mix required to keep things running has changed over time.

That would be easier said than done for me. I'm actually using ReStore, so the
management of statements is going on without any involvement on my part. (And
I'd just as soon keep it that way :)I did do some "management" of the problem a
little while ago. By adding an explicit #disconnect on the ReStore after a
TestCase was done, the problem, which had been quite prevalent, seemed to go
away. It bothered me that leaving it to finalization would have a problem, but I
moved on. But when the problem resurfaced anyway, I dug into it.

In light of what I figured out, the change in failure rate makes sense. With an
explicit #disconnect, the chances are that the DBStatement hasn't yet been GCed,
and so the DBConnection is able to explicitly free it. With the #disconnect
happening as a result of DBConnection finalize, the chances of the statement and
connection getting finalized at roughly the same time goes way up. It which case
the order of their finalization will govern whether or not the memory protection
fault occurs.

     [snip]

I'm quite confident of my findings. Between the debug trace log of all the
MyODBC calls, and having instrumented DBStatement>>allocatedHandle,  basicFree
and DBConnection>>disconnect and invalidateAllStmts, everything points to the
above sequence as being what happens when the protection problem occurs.

Still looking for inspiration though on how to fix this, without keeping strong
references to the statements around or having to actively manage the statements
life cycle.

Thanks,
-Bill

-------------------------------------------
Bill Dargel            [hidden email]
Shoshana Technologies
100 West Joy Road, Ann Arbor, MI 48105  USA

Bill,

> > My "solution" has long been to clean up statements as I used them, and
the
> > exact mix required to keep things running has changed over time.
>
> That would be easier said than done for me. I'm actually using ReStore, so
the
> management of statements is going on without any involvement on my part.
(And
> I'd just as soon keep it that way :)

Understood.  I have some fairly horrible but well-tested code that does
enough to get by, and is otherwise undefendable.  Even so, I'd be glad to
see a real fix.

Can you transplant your repeatable failure case?  ReStore is a commercial
product, but maybe OA has a copy or perhaps the vendor could take pitty on
us long enough to let them debug the problem.

Have a good one,

Bill

--
Wilhelm K. Schwab, Ph.D.
[hidden email]

Bill Dargel wrote:

> Still looking for inspiration though on how to fix this, without keeping
strong
> references to the statements around or having to actively manage the
statements
> life cycle.

This is clearly something for OA to fix, but -- in the interim -- would
something like changing DBConnection>>close so that it also did:

    DBAbstractStatement primAllSubinstances do:
        [:each | each parent == self ifTrue: [each reset]].

do the trick ?

    -- chris

Chris Uppal wrote:

> This is clearly something for OA to fix, but -- in the interim -- would
> something like changing DBConnection>>close so that it also did:
>
>     DBAbstractStatement primAllSubinstances do:
>         [:each | each parent == self ifTrue: [each reset]].
>
> do the trick ?

Cool! :-)

I had second thoughts for a while. Wouldn't the DBStatements that had been
removed from the WeakSet also not show up in the #primAllSubinstances? But I
tried it out anyway.

Caught a case in #disconnect where the statements WeakSet had one statement in
it. That statement had been freed just before during the DBStatement finalize,
and showed a nil handle. A second statement that was not in the WeakSet had
its finalize (and erroneous #free) happen subsequently. Both of the statements
did show up in the #primAllSubinstances filtered by the parent matching.

So it seems that the moral of the story is that the order in which objects'
weak references disappear and the order in which their finalize methods are
called can even be in opposite order!

I decided to replace relying on the WeakSet with what you suggested, only
explicitly freeing the statements. So I took out:
    self invalidateAllStmts.
and and replaced it by:
    DBAbstractStatement  do:
        [:each | each parent == self ifTrue: [each free]].

Seems to be working okay. As long as the #primAllSubinstances can find all the
instances, including those that have lost all strong references given that
they have a #finalize that's still pending, it should work.

I'll leave it to OA's judgment as to whether this is the right solution or
not.

-------------------------------------------
Bill Dargel            [hidden email]
Shoshana Technologies
100 West Joy Road, Ann Arbor, MI 48105  USA

Bill Schwab wrote:

> Can you transplant your repeatable failure case?  ReStore is a commercial
> product, but maybe OA has a copy or perhaps the vendor could take pitty on
> us long enough to let them debug the problem.

It's not exactly that repeatable. If everything is _exactly_ the same, it can be
fairly repeatable. But that's a big caveat. The order in which things get GC'd
and finalized depend on global image state as much as any TestCase>>setUp. If
necessary, I can try and pull something out into a clean image, but hopefully
I've provided enough analysis of what's going on so that OA can see what to do
about it.

-------------------------------------------
Bill Dargel            [hidden email]
Shoshana Technologies
100 West Joy Road, Ann Arbor, MI 48105  USA

Bill,

This issue has been recorded as #1072. While not the correct fix, it seems
that Chris's suggestion should work to keep you going until we come up with
the solution.

Best Regards,

Andy Bower
Dolphin Support
http://www.object-arts.com
---
Are you trying too hard?
http://www.object-arts.com/Relax.htm
---

"Bill Dargel" <[hidden email]> wrote in message
news:[hidden email]... I
> tried it out anyway.
>
> Caught a case in #disconnect where the statements WeakSet had one
statement in
> it. That statement had been freed just before during the DBStatement
finalize,
> and showed a nil handle. A second statement that was not in the WeakSet
had
> its finalize (and erroneous #free) happen subsequently. Both of the
statements
> did show up in the #primAllSubinstances filtered by the parent matching.
>
> So it seems that the moral of the story is that the order in which
objects'
> weak references disappear and the order in which their finalize methods
are the

Andy Bower wrote:

> This issue has been recorded as #1072. While not the correct fix, it seems
> that Chris's suggestion should work to keep you going until we come up with
> the solution.

Thanks Andy.

John Aspinall from Solutions Software, the maker of ReStore, sent me some
workspace code where he saw the error one time. I simplified and reworked it so
that it fails virtually every time:

c := DBConnection new.
c dsn: 'mysql'.
c connect.
c exec: 'select * from person'.
[c statements isEmpty] whileFalse: [Processor sleep: 10].
c disconnect.

It's basically monitoring the WeakSet of statements, watching for the statement
to get GC'd and then disconnecting at the worst possible time.

-------------------------------------------
Bill Dargel            [hidden email]
Shoshana Technologies
100 West Joy Road, Ann Arbor, MI 48105  USA

"Bill Dargel" <[hidden email]> wrote in message
news:[hidden email]...
>...[snip]...
> I decided to replace relying on the WeakSet with what you suggested, only
> explicitly freeing the statements. So I took out:
>     self invalidateAllStmts.
> and and replaced it by:
>     DBAbstractStatement  do:
>         [:each | each parent == self ifTrue: [each free]].
>
> Seems to be working okay. As long as the #primAllSubinstances can find all
the
> instances, including those that have lost all strong references given that
> they have a #finalize that's still pending, it should work.
>
> I'll leave it to OA's judgment as to whether this is the right solution or
> not.

A preferable solution would seem to be to to delegate the task of freeing
the statement handle (in DBAbstractStatement>>basicFree) to the parent
connection. This would allow the connection to ignore the free if it has
itself been closed.

Incidentally, just in case you are sending #disconnect explicitly, I should
point out that it is not a good idea to do that since it will leave the
connection in an inconsisitent state. DBConnections do not distinguish
between the states of having a handle allocated, and being connected. When a
connection is opened, the handle is allocated and connection attempted,
should the connect fail then the handle is released. The availability of a
non-null handle is used by DBConnection to determine whether it is connected
or not. #disconnect is a private method that calls SQLDisconnect, but it
does not free the handle, so the DBConnection will think it is still
connected. Explicit disconnection must be through DBConnection>>close, which
also frees the connection handle.

Anyway I offer the attached as a preliminary patch. It has not received much
testing yet, but all our existing SUnits for the DB package pass with this
installed.

Regards

Blair

---------------------
!DBConnection methodsFor!

freeStatement: aDBAbstractStatement
 | ret |
 handle isNull ifTrue: [^self].
 ret := ODBCLibrary default sqlFreeHandle: SQL_HANDLE_STMT
    handle: aDBAbstractStatement handle.
 ret ~= SQL_SUCCESS
  ifTrue: [DBError signalWith: (aDBAbstractStatement exceptionDetails:
ret)]! !
!DBConnection categoriesFor: #freeStatement:!helpers!private! !

!DBAbstractStatement methodsFor!

handle
 ^handle!

basicFree
 "Private - Free up all ODBC resources."

 parent freeStatement: self! !
!DBAbstractStatement categoriesFor: #handle!accessing!private! !
!DBAbstractStatement categoriesFor:
#basicFree!private!realizing/unrealizing! !

Blair McGlashan wrote:

> A preferable solution would seem to be to to delegate the task of freeing
> the statement handle (in DBAbstractStatement>>basicFree) to the parent
> connection. This would allow the connection to ignore the free if it has
> itself been closed.

I like it. The simple solution. Much easier than worrying about the vagaries in
the ordering of finalizations and loss of weak references!

Thanks for pointing this out. One sends the main ReStore class #disconnect, so
it's easy enough to forget to switch mental gears when moving to the underlying
DBConnection level. Indeed, in checking, I found one place where I sent
DBConnection #disconnect, when it really should have been #close. (Didn't really
matter, since the instance variable holding the DBConnection was set to nil at
that point and wouldn't have been reused, ... but still good to clean it up)

> Anyway I offer the attached as a preliminary patch. It has not received much
> testing yet, but all our existing SUnits for the DB package pass with this
> installed.

I'm using the patch and will let you know of any issues that turn up.

Thanks,
-Bill

-------------------------------------------
Bill Dargel            [hidden email]
Shoshana Technologies
100 West Joy Road, Ann Arbor, MI 48105  USA

Issue #1072 - "Occassional GPF caused by DBConnection finalization order" is
listed as being fixed in PL2.
http://www.object-arts.com/Lib/Update/Dolphin/5.0/Professional/PL2.htm
But I don't see any changes to the Database Connection Base package. In
particular the change to DBAbstractStatement>>basicFree and the addition of
DBAbstractStatement>>handle and DBConnection>>freeStatement.

Were they missed?

thanks,
-Bill

Blair McGlashan wrote:


-------------------------------------------
Bill Dargel            [hidden email]
Shoshana Technologies
100 West Joy Road, Ann Arbor, MI 48105  USA

"Bill Dargel" <[hidden email]> wrote in message
news:[hidden email]...
> Issue #1072 - "Occassional GPF caused by DBConnection finalization order"
is
> listed as being fixed in PL2.
> http://www.object-arts.com/Lib/Update/Dolphin/5.0/Professional/PL2.htm
> But I don't see any changes to the Database Connection Base package. In
> particular the change to DBAbstractStatement>>basicFree and the addition
of
> DBAbstractStatement>>handle and DBConnection>>freeStatement.
>
> Were they missed?

Yes, the patch is indeed missing. We had a major disaster with the loss of a
disk on our main server recently, and had to restore our source code
database from an older backup than we would have liked. Its possible it got
lost then. I've re-opened the issue.

Thanks for the report.

Blair