Code review for thread safeness

Code review for thread safeness

[Marius Vollmer]

Hi,

as far as I am concerned, the last larger thing holding up a Guile 1.8
release is a code review with the aim of making libguile thread safe.

I have replaced all SCM_DEFER_INTS, SCM_ALLOW_INTS calls with
SCM_CRITICAL_SECTION_START/END.  We need to check all occurences
whether they are really used correctly, as explained in the ref
manual.  Basically, there must be no non-local exits between
SCM_CRITICAL_SECTION_START and SCM_CRITICAL_SECTION_END.  If there
might be non-local exits, scm_frame_critical_section can be used
instead.

(Right now, Guile aborts when a non-local exit does in fact happen
between SCM_CRITICAL_SECTION_START and _END.)

Also, there are probably many more places that need to become critical
sections.  We need to find them and use SCM_CRITICAL_SECTION_START/END
or scm_frame_critical_section, as appropriate.

I will release 1.7.2 with a warning that the code review has not been
done yet and then start the review itself.

Any helpers?  Please reply here so that we can coordinate.

-- 
GPG: D5D4E405 - 2F9B BCCC 8527 692A 04E3  331E FAF8 226A D5D4 E405


_______________________________________________
Guile-devel mailing list
Guile-devel@gnu.org
http://lists.gnu.org/mailman/listinfo/guile-devel

Re: Code review for thread safeness

[Kevin Ryde]

Marius Vollmer <mvo@zagadka.de> writes:
>
> If there might be non-local exits, scm_frame_critical_section can be
> used instead.

If an error occurs and a lazy-catch executes, is the mutex unlocked
for that handler?

> Also, there are probably many more places that need to become critical
> sections.  We need to find them and use SCM_CRITICAL_SECTION_START/END
> or scm_frame_critical_section, as appropriate.

The ones I've spotted are (I might have posted this before),

	gethostbyname
        getpwuid
	setpwent (etc)
	getgrgid
	setgrent (etc)
	setlocale
	scm_mem2symbol 

or scm_i_mem2symbol or whatever it is now.  I probably won't get a
chance to actually do anything about these.


_______________________________________________
Guile-devel mailing list
Guile-devel@gnu.org
http://lists.gnu.org/mailman/listinfo/guile-devel

Re: Code review for thread safeness

[Marius Vollmer]

Kevin Ryde <user42@zip.com.au> writes:

> Marius Vollmer <mvo@zagadka.de> writes:
>>
>> If there might be non-local exits, scm_frame_critical_section can be
>> used instead.
>
> If an error occurs and a lazy-catch executes, is the mutex unlocked
> for that handler?

No.  Good point.  Hmm.  I haven't thought about lazy catches at all,
yet...  They are like asyncs but can not be blocked.  But, they are
not as much uncontrolled as asyncs.

The important point here is that a critical section is not reentered
when it is not designed to allow this.  When a lazy handler tries to
reenter it, an error is signalled when scm_frame_critical_section is
used with a non-recursive mutex.  I think this is appropriate.

> The ones I've spotted are (I might have posted this before),
>
> 	gethostbyname
>         getpwuid
> 	setpwent (etc)
> 	getgrgid
> 	setgrent (etc)
> 	setlocale
> 	scm_mem2symbol 
>
> or scm_i_mem2symbol or whatever it is now.  I probably won't get a
> chance to actually do anything about these.

Thanks, I'll check them.

-- 
GPG: D5D4E405 - 2F9B BCCC 8527 692A 04E3  331E FAF8 226A D5D4 E405


_______________________________________________
Guile-devel mailing list
Guile-devel@gnu.org
http://lists.gnu.org/mailman/listinfo/guile-devel

Re: Code review for thread safeness

[Ken Raeburn]

On Mar 7, 2005, at 16:36, Kevin Ryde wrote:
>> Also, there are probably many more places that need to become critical
>> sections.  We need to find them and use SCM_CRITICAL_SECTION_START/END
>> or scm_frame_critical_section, as appropriate.
>
> The ones I've spotted are (I might have posted this before),
>
> 	gethostbyname
>         getpwuid

If at all possible, this code should be switched to use get*by*_r, or 
in the gethostby* cases, perhaps getaddrinfo and getnameinfo.  Marking 
it as a critical section for Guile won't prevent some other thread that 
is doing no Guile stuff at all from calling gethostbyname or whatever 
around the same time.

Ken


_______________________________________________
Guile-devel mailing list
Guile-devel@gnu.org
http://lists.gnu.org/mailman/listinfo/guile-devel

Re: Code review for thread safeness

[Marius Vollmer]

Ken Raeburn <raeburn@raeburn.org> writes:

> If at all possible, this code should be switched to use get*by*_r, or
> in the gethostby* cases, perhaps getaddrinfo and getnameinfo.

Yes, definitely.  Could you help with that?


_______________________________________________
Guile-devel mailing list
Guile-devel@gnu.org
http://lists.gnu.org/mailman/listinfo/guile-devel

Re: Code review for thread safeness

[Andreas Rottmann]

Marius Vollmer <marius.vollmer@uni-dortmund.de> writes:

> Ken Raeburn <raeburn@raeburn.org> writes:
>
>> If at all possible, this code should be switched to use get*by*_r, or
>> in the gethostby* cases, perhaps getaddrinfo and getnameinfo.
>
> Yes, definitely.  Could you help with that?
>
FWIW, I just stole some code from GNet (http://www.gnetlibrary.org)
for another project. You might want to have a glance at how they do
this stuff, since it seems there are different flavours of
gethostbyname_r (glibc, HPUX, Solaris). The relevant code is in
inetaddr.c and configure.ac (detection). GNet is LGPL-licenced BTW.

Rotty
-- 
Andreas Rottmann         | Rotty@ICQ      | 118634484@ICQ | a.rottmann@gmx.at
http://yi.org/rotty      | GnuPG Key: http://yi.org/rotty/gpg.asc
Fingerprint              | DFB4 4EB4 78A4 5EEE 6219  F228 F92F CFC5 01FD 5B62

It's *GNU*/Linux dammit!



_______________________________________________
Guile-devel mailing list
Guile-devel@gnu.org
http://lists.gnu.org/mailman/listinfo/guile-devel

Re: Code review for thread safeness

[Ken Raeburn]

Oh, how annoying.  In going back over some old email messages in my 
way-too-full mailbox, I just noticed these replies to my message that 
I'd never read!  I apologize for not replying before.

On Mar 9, 2005, at 11:23, Andreas Rottmann wrote:
> Marius Vollmer <marius.vollmer@uni-dortmund.de> writes:
>> Ken Raeburn <raeburn@raeburn.org> writes:
>>> If at all possible, this code should be switched to use get*by*_r, or
>>> in the gethostby* cases, perhaps getaddrinfo and getnameinfo.
>> Yes, definitely.  Could you help with that?
> FWIW, I just stole some code from GNet (http://www.gnetlibrary.org)
> for another project. You might want to have a glance at how they do
> this stuff, since it seems there are different flavours of
> gethostbyname_r (glibc, HPUX, Solaris). The relevant code is in
> inetaddr.c and configure.ac (detection). GNet is LGPL-licenced BTW.

I've spent some time dealing with getaddrinfo/gethostbyname_r/etc for 
work, and found this to be a rather annoying morass to dive into.  
There are at least three variants of gethostbyname_r among the systems 
I'm supporting, and some (even modern systems like NetBSD and Mac OS X) 
don't have it at all; almost all of the systems have getaddrinfo, 
though different implementations have different minor bugs.  I've also 
gone through writing code to determine which flavors of which routines 
are available, and implementing a wrapper to deal with all the 
variants.

In the long term, I think getaddrinfo is what we want to be using.  But 
for the short term, hacking up the gethostby* routines to use the _r 
versions when they're available shouldn't be that hard.  I'll look into 
it.

(Oh, eww, we've got 'gethostent' support too.  Bleah.  Well, glibc at 
least seems to have a gethostent_r...)

Ken


_______________________________________________
Guile-devel mailing list
Guile-devel@gnu.org
http://lists.gnu.org/mailman/listinfo/guile-devel

Re: Code review for thread safeness

[Marius Vollmer]

Kevin Ryde <user42@zip.com.au> writes:

> Marius Vollmer <mvo@zagadka.de> writes:
>>
>> If there might be non-local exits, scm_frame_critical_section can be
>> used instead.
>
> If an error occurs and a lazy-catch executes, is the mutex unlocked
> for that handler?

No, the handlers of the lazy-catch execute in the same dynamic context
as the (throw ...) that started them.

I guess you are worried about what happens when the lazy handler tries
to enter the critical section (or another one that uses the same
mutex) again.  I think this is covered by this from the manual:

     When the current thread reenters the critical section anyway, the
     kind of MUTEX determines what happens: When MUTEX is recursive,
     the reentry is allowed.  When it is a normal mutex, an error is
     signalled.

-- 
GPG: D5D4E405 - 2F9B BCCC 8527 692A 04E3  331E FAF8 226A D5D4 E405


_______________________________________________
Guile-devel mailing list
Guile-devel@gnu.org
http://lists.gnu.org/mailman/listinfo/guile-devel