> 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
>
> --- 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);
> }
> }
>
