static current session variable

[Derek Atkins]

Neil,

I hope you don't mind that I responded to the list -- this is
something I think everyone should be able to see.  It's useful
information and gives us the chance to archive the information for
posterity.  A couple years from now it might be useful to some other
developer ;)

Neil Williams <linux at codehelp.co.uk> writes:

> I'm sorry if this is basic stuff, I just want to get it checked:
>
> Should the static current_session variable be static in qofsession.c?

Techincally... no.  The current_session should be an application
thread-specific data object instead of hard-coded in the qof library.
It's that way for historical purposes, and eventually we should change
it so there isn't a "current session" in QOF.

But....  that would require changing every API everywhere in the code
to always have a session context available.  That would be a major
rototill of the code -- something that should eventually happen but
not something I suggest we start now.  It would be a LOT of work to
rototill that.  It would effectively touch almost every API in both C
and Scheme to fix it..  Which is why it hasn't been done.

> If two processes are using the QOF library and each call
> qof_session_set_current_session()
> with a different session, and later one of them calls
> qof_session_get_current_session(),
> could the wrong session be returned?

Two PROCESSES?  No.  The static variable is in the data portion of the
library, not the text portion of the library.  The data portion is
copy-on-write cache, so when the PROCESS makes a change, it will
overwrite the process-specific copy-on-write version of the variable.

HOWEVER, it means two THREADS in the _same_ process will share the
data.  That's why you don't want data-context to be static, because
you might want different threads working on different data sets.

> Should current_session be an *application* variable, not a library one?

Eventually, yes..  But I wouldn't worry about it yet.

> What about the static registered object hash table in qofclass.c ?
> static GHashTable *classTable = NULL;
> and the modules_list in qofobject.c:
> static GList *object_modules = NULL;

These are okay to remain static objects in the library for the above
reason.  You're not going to need two threads in the same app to use a
different set of class objects.  Those are process-wide settings, so a
static set is just fine.

> I've tested this one and it does seem to work. I can call cashutil --shell (so 
> that it's objects are registered and the process continues to run) and then 
> pilot-qof -l in another terminal which registers it's own objects and prints 
> out the list using qof_object_foreach_type(). That simply iterates through 
> the object_modules GList in the QOF library, which is a static. Why wouldn't 
> I get the cashutil list? I don't, I get what we should get, the pilot-qof 
> list in pilot-qof and the cashutil list in cashutil.
>
> Why does this differ to how the mergeData had to be handled?

The current_session variable is the way it is for historical reasons,
not because it is correct.  As (I hope) I've shown, the way it's done
is wrong.  New code shouldn't use static variables for data context.
(Process context is okay, like those module/object lists).  The
problem is that you cannot have two threads in the same process that
have different sessions.  For example, wouldn't it be nice to have one
gnucash app that has two books open at the same time in two top-level
windows?

Unfortunately we cannot have that with the current implementation of
the static current_session.

The same argument can be said about mergeData -- it would be nice to
have two threads acting on separate merges.  Perhaps the user has two
merge processes going at the same time.  That's why mergeData couldn't
be static.

-derek

-- 
       Derek Atkins, SB '93 MIT EE, SM '95 MIT Media Laboratory
       Member, MIT Student Information Processing Board  (SIPB)
       URL: http://web.mit.edu/warlord/    PP-ASEL-IA     N1NWH
       warlord at MIT.EDU                        PGP key available