Re: Database connections in Versaplex
Avery Pennarun
include all the main Versaplex developers anyway :)
Thanks for the clear description of the problem. From that, I think I
can explain how to solve it cleanly.
> When a client connects to Versaplexd, it's not necessarily obvious
> which database connection to use. The non-Schemamatic code currently
> calls VxSqlPool.GetConnection(), which looks at who sent the request
> (which it finds out by asking WvDbusd for either the username or SSL
> cert fingerprint associated with the connection), then it looks in the
> config file to see what connection string should be used for that
> client. Then it (naturally enough) connects using that string, runs
> the query or whatever, and closes the connection.
>
> At first, Schemamatic used exactly this mechanism, since it just
> called the existing Versaplex internal methods to run queries. Then I
> split Schemamatic out into a somewhat more independent section, so
> that wasn't going to work any more. At first, things worked pretty
> much the same way, except I had to take the connection string and do
> all the connection and query-running myself. This was a bit
> suboptimal, because it tends to be tedious and repetitive, and doesn't
> take advantage of all the stuff WvDbi brings to the table (which the
> rest of Versaplex also ignores).
>
> So then, WvDbi. The best way to use WvDbi is to call the methods that
> return iterators, to avoid loading tons of results into memory.
So far, so good.
> But
> the Schemamatic code in question also needs to convert any received
> SqlExceptions into VxSqlExceptions,
I think this is the first place where we're introducing problems. Why
is it *schemamatic's* job to turn SqlExceptions into VxSqlExceptions?
The Vx in the name implies to me that it should be Versaplex's job,
and Schemamatic isn't even always part of Versaplex.
I saw an interview with Anders Hejlsberg (inventor of Delphi and C#)
one time where he was discussing exceptions, and his advice has
changed the whole way I think about exceptions. Short version: the
ratio of try-finally to try-catch in your code should be something
like 10:1. (C++ doesn't have try-finally, but it has working
destructors, which are even better, and that implies that most of your
C++ code should *never* explicitly deal with exceptions.)
Now, how do you follow this advice?
There are two important factors:
1. Your code should be throwing exceptions only in actual exceptional cases.
2. Your code should deal with most exceptional cases at the UI level;
if this isn't the case, your exceptions are not exceptional enough, so
go back to #1.
The way I translate this into real programs is:
1. When idiotic libraries throw exceptions for idiotic reasons (like
failing to convert a string to an int), write a wrapper function that
catches the exception and does something sensible instead. (atoi()
returning 0 is pefectly fine behaviour, IMHO; have you ever seen a C
programmer need to write an error-throwing replacement?) In the ideal
world, the exception would never be thrown in the first place, but
this is the best we can do when using other people's code.
2. When real exceptions happen throughout the code - such as an
SqlException - don't catch them. "Never check for an error you don't
know how to handle" is also time-tested good programming advice. On
the other hand, Versaplex wants to *propagate* exceptions cleanly up
the call stack, over dbus, to its original caller. To do that, the
*top-level* handler for the outermost event loop in Versaplex should
catch SqlExceptions and convert them to the appropriate thing. That's
just one try-except, isolated in one place, and it's doing an actual
useful job.
> the returned iterators can't
> outlive the WvDbi that created them, and, as we discovered today, we
> can't leave WvDbi objects lying around without cleaning up. So if I
> want to keep making lots of WvDbi objects, my choices seem to boil
> down to:
I think there's another misconception leading to this problem: you
seem to be confusing the cost of *keeping around* WvDbi objects
(keeping a pointer to an idle one) with the cost of *hoarding* WvDbi
objects (keeping one and not letting others use it). The proper
lifetime for a WvDbi object is for the entire length of the session
that is using that database.
And that WvDbi object can have as many simultaneous referencers as we
want during that time, as long as only one of those referencers wants
to do a request at the same time. This sounds restrictive, but is
actually really easy to enforce, because a) we're not using threads
(except potentially at the per-session level, outside the scope of the
WvDbi), and b) WvDbi returns mostly iterators, so the only way you'd
be doing multiple requests at once would be if you have nested loops
of database queries, which there is no particular reason to have (as
far as I know) in Schemamatic or Versaplexd.
What's the length of a session?
a) In a standalone Schemamatic application, the lifetime of the UI, or
whenever the UI switches from one database to another, if it offers
that.
b) In Versaplex (which is effectively connectionless), exactly one
request-response cycle. Conveniently, right after the request is
received is when you figure out which database you want to connect to,
so that's the perfect time to make the WvDbi object for your session.
After that, don't pass database connection strings around to anybody;
pass your WvDbi object around. Then anybody who *receives* a WvDbi
doesn't have to clean it up or put it in a using(), because they can
assume that the session is handling that with a using() or whatever at
the outermost level, around the same place as those exceptions are
being caught. Of course each user of WvDbi should make sure to
dispose of their iterators correctly, but foreach() does that for you.
As long as you always use foreach (var x in dbi.select(whatever)), or
else dbi.select(whatever).ToArray() (which uses foreach internally),
it's all good.
...
What this all comes down to is that C#'s stupid lack of sensible
destructors is indeed incredibly stupid, so we have to design around
it. The key things to realize are:
1. using() turns nonsense destructors into sensible destructors. If
all variables were just wrapped in using(), we'd have C++ destructor
behaviour. (We would also then have to implement refcounting smart
pointers, etc, and using() those would increment/decrement the
refcount on the inner object rather than always immediately disposing
it.)
2. foreach() implicitly does using() and is quite often more appropriate.
3. Almost no resources actually need to be explicitly destroyed, since
the most common resource, memory, is cleaned up automatically. The
problem is that some resources *do* need to be destroyed, and if you
own one, then *you* inherit that property, and so on out to the top
level object. Basically if your object owns an IDisposable object, it
*needs* to be IDisposable. But if it only has an IDisposable on its
stack, it can handle it with using() for foreach(). The workaround
for this annoyance is to just do everything you possibly can to avoid
*owning* IDisposable objects outside the stack.
See how we've followed this advice above:
1. We have using(WvDbi dbi) at the toplevel, so it gets destroyed
safely in the normal case and when there are exceptions.
2. We use foreach() for all the actual datasets, so they get destroyed
safely in the normal case and when there are exceptions.
3. Nobody ever *owns* an IDisposable object (WvDbi or dataset) as an
object member; it's always either on the stack, or a non-owning
reference.
4. WvDbi itself has no escape; it has to be IDisposable since it owns
an IDisposable connection. But that's only one object in the whole
big scheme, so that's pretty reasonable. (And the C# creators would
probably argue that having IDisposable in this one place is a lot
cheaper than having deterministic destructors *everywhere*.)
As long as you follow these rules, C# code should be relatively under control.
Hmm, perhaps I should write a journal entry...
Have fun,
Avery