apr-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "William A. Rowe, Jr." <wr...@apache.org>
Subject RE: cvs commit: apr/file_io/win32 filedup.c open.c readwrite.c
Date Tue, 29 Oct 2002 17:47:33 GMT
At 11:32 AM 10/29/2002, Bill Stoddard wrote:

>> At 06:00 AM 10/29/2002, Bill Stoddard wrote:
>>
>> >> And it has a flaw if we are logging errors from the parent, the old
>> >> race condition still remains.  It gets much worse if we factor in
>> >> the possibility of two child worker processes accessing the file
>> >> at once.  Thread locks are strictly intraprocess.
>> >
>> >The LockFile calls will protect against cross process updates to the file
>> >(when in APR_APPEND mode). The thread locks are to work around what
>> >appears to be a bug in the Windows LockFile implementation. LockFile
>> >can deadlock with WriteFile and I expect it is only an issue when multiple
>> > threads in a single process are accessing LockFile.
>>
>> I wasn't reading closely enough, shame on me :-)
>
>Hey, I've never been guilty of that! Keep on your toes boy. ;-) Seriously, I am
>really glad you are reviewing this code. Thanks.
>
>>
>> I see no problem with ignoring the pOverlapped when choosing to lock
>> the file, and it makes the code simpler.
>>
>> I'll commit the following patch a bit later today, unless you know
>> of some restriction that I'm unaware of.
>>
>> Bill
>
>The reason for locking the file is to mutex the file pointer. A file opened for
>overlapped i/o does not have a file pointer.
>I'm not aware of any restrictions but I've not tested this patch and I think the
>results will be less than predictable. For instance, we lock the file, call
>WriteFile and get back IO_PENDING, then we unlock the file. Maybe the i/o
>occured by the time we released the lock. Or maybe not.  The function is broken,
>with or without this patch when used for writing to files opened for overlapped
>i/o. I'm not sure the best way to fix it (or even if we should fix it). I'm -0
>on this patch.

Bill, now you aren't reading :-)
Please take a moment and review apr_file_seek()...

The code never sets the file pointer through the win32 API.  There is
no bug.  As long as we hold the file lock, the size won't change.  The
WriteFile uses the filePtr value for the overlapped value.

apr_file_seek, for XTHREAD/overlapped files, simply updates filePtr based
on your request, relative to the old filePtr value :-)  So we still want the
file lock, to hold the length of the file constant, until our file write completes.

Hope that clears things up.

Bill

>> --- srclib/apr/file_io/win32/readwrite.c        29 Oct 2002 02:19:50
>> -0000      1.71
>> +++ srclib/apr/file_io/win32/readwrite.c        29 Oct 2002 17:12:13 -0000
>> @@ -308,12 +308,10 @@
>>              apr_status_t rc;
>>              if (thefile->append) {
>>                  apr_thread_mutex_lock(thefile->mutex);
>> -                if (!thefile->pOverlapped) {
>> -                    rc = apr_file_lock(thefile, APR_FLOCK_EXCLUSIVE);
>> -                    if (rc != APR_SUCCESS) {
>> -                        apr_thread_mutex_unlock(thefile->mutex);
>> -                        return rc;
>> -                    }
>> +                rc = apr_file_lock(thefile, APR_FLOCK_EXCLUSIVE);
>> +                if (rc != APR_SUCCESS) {
>> +                    apr_thread_mutex_unlock(thefile->mutex);
>> +                    return rc;
>>                  }
>>                  rc = apr_file_seek(thefile, APR_END, &offset);
>>                  if (rc != APR_SUCCESS) {
>> @@ -328,9 +326,7 @@
>>              rv = WriteFile(thefile->filehand, buf, *nbytes, &bwrote,
>>                             thefile->pOverlapped);
>>              if (thefile->append) {
>> -                if (!thefile->pOverlapped) {
>> -                    apr_file_unlock(thefile);
>> -                }
>> +                apr_file_unlock(thefile);
>>                  apr_thread_mutex_unlock(thefile->mutex);
>>              }
>>          }
>>


Mime
View raw message