Created attachment 465 [details]
initial patch (NOTE: against 2.43 and not HEAD, needs to be revised)

Note that the initial patch is against 2.43 and not HEAD.  I need to revise it.

I also added a Cc: for Craig since it was his code originally.

Subject: Re: [SAdev]  New: fix locking on Win32 in DBBasedAddrList 


btw, it gets worse -- flock() does not work on some versions of Windows (I
think 95, and 98). cf:

http://www.perlmonks.com/index.pl?node=mutual%20exclusion%20and%20file%20locking%20on%20windows

But I think it gives a nice clear compile-time error in that case, so 

	eval { if (0) { flock(); } 1; }

should be a way to test it.  (could someone check that on a win32 system?)

"NFS-safety" per se should not be a necessity on win32, but some form of
local locking should be used.

How's about flock() for recent win32s, and *no* locking for win95 and
98?  They're now officially obsolete anyway.

(PS: Dan, the openhandhome SpamAssassin on win32 FAQ might be useful ;)

--j.

assigning to myself and giving 2.50 tag, I already have a fix that I'm
testing

Created attachment 577 [details]
proposed patch to address Win32 locking

I am attaching a proposed fix based on mkdir() locking since link() does not
work on Win32 and flock() supposedly does not work on Win95.  My second choice
was to use sysopen with O_CREAT|O_EXCL.

Comments?

Subject: Re: [SAdev]  fix locking on Win32 in DBBasedAddrList 


> I am attaching a proposed fix based on mkdir() locking since link() does
> not work on Win32 and flock() supposedly does not work on Win95.  My
> second choic e was to use sysopen with O_CREAT|O_EXCL.

Looks nice -- assuming mkdir() is atomic on Win32 filesystems ;)

Dan, want to add this to CVS?  seems OK to me.

(mind you, I don't have a clue if mkdir() is atomic on all win32 filesystems,
but if your code implies it is, I'll trust it ;)

Subject: Re: [SAdev]  fix locking on Win32 in DBBasedAddrList

jm@jmason.org writes:

> Dan, want to add this to CVS?  seems OK to me.
> 
> (mind you, I don't have a clue if mkdir() is atomic on all win32
> filesystems, but if your code implies it is, I'll trust it ;)

Not quite yet.  I'm not quite certain enough about mkdir() semantics on
Win32 although I think it's probably okay.  I may end up using a
semaphore which will add a dependency, but I'd rather be conservative
and be sure it works right.

Subject: Re: [SAdev]  fix locking on Win32 in DBBasedAddrList 


> Not quite yet.  I'm not quite certain enough about mkdir() semantics on
> Win32 although I think it's probably okay.  I may end up using a
> semaphore which will add a dependency, but I'd rather be conservative
> and be sure it works right.

as far as I can see from MSDN and the perl sources for 5.8.0, it should
be atomic.

But I agree -- using a win32 extension module may be safer; MSDN in
particular never states "mkdir() is atomic on such-and-such filesystems".

--j.

Subject: Re:  fix locking on Win32 in DBBasedAddrList

With the way the new pluggable filesystem layer works in XP, I doubt 
very much that this is in fact guaranteed -- it's probably up to the 
implementor to decide this.

C

On Wednesday, January 15, 2003, at 12:44 PM, 
bugzilla-daemon@hughes-family.org wrote:

> But I agree -- using a win32 extension module may be safer; MSDN in
> particular never states "mkdir() is atomic on such-and-such 
> filesystems".

Dan -- just checked in an abstraction of locking into a separate
class, loaded by OS -- so we can now throw any kidn of Win32-specific
wierdness into the Win32Locker class ;)

For now, I've put in your patch's code.  But I suggest using a proper
Win32 API locking call instead...

I think this version is slightly better than using mkdir().  I don't think
we should ship the mkdir() version with 2.50.  Will attach in a moment.

The patch converts the locking routine to use sysopen().  I would attach
a slightly higher confidence value to this new code.  The patch also adds
some additional debugging code (mostly pids), comments, removes an unnecessary
dependency on Fcntl in the Unix code, etc.

If it looks good to you, I'll go ahead and check it into SA.

Created attachment 631 [details]
new version of Win32 locking

I checked the new sysopen-based code into CVS.  I'm going to close this bug
since Win32 locking does work, but I plan on opening a new one since it doesn't
work over NFS.

Who the heck uses NFS on windows?  I think if you're doing that, you can more or less expect locking (and lots of other stuff probably) is going to be failing...If you reopen it Daniel, make sure to set the priority super-low.

Subject: Re:  fix locking on Win32 in DBBasedAddrList

craig@hughes-family.org writes:

> Who the heck uses NFS on windows?  I think if you're doing that, you
> can more or less expect locking (and lots of other stuff probably) is
> going to be failing...If you reopen it Daniel, make sure to set the
> priority super-low.

Well, Windows users do use network filesystems (CIFS shares) that may
have problems with the current lock code.  For example, I've seen people
put their Documents folder on remote Network Appliance servers and that
might cause problems for SpamAssassin.