Return-Path: Delivered-To: apmail-apr-dev-archive@apr.apache.org Received: (qmail 53174 invoked by uid 500); 29 Oct 2002 17:37:53 -0000 Mailing-List: contact dev-help@apr.apache.org; run by ezmlm Precedence: bulk List-Post: List-Help: List-Unsubscribe: List-Subscribe: Delivered-To: mailing list dev@apr.apache.org Received: (qmail 53143 invoked from network); 29 Oct 2002 17:37:50 -0000 From: "Bill Stoddard" To: "APR Development List" Subject: RE: cvs commit: apr/file_io/win32 filedup.c open.c readwrite.c Date: Tue, 29 Oct 2002 12:32:44 -0500 Message-ID: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit X-Priority: 3 (Normal) X-MSMail-Priority: Normal X-Mailer: Microsoft Outlook IMO, Build 9.0.2416 (9.0.2911.0) X-MimeOLE: Produced By Microsoft MimeOLE V6.00.2600.0000 In-Reply-To: <5.1.0.14.2.20021029110726.024dada8@pop3.rowe-clan.net> Importance: Normal X-Spam-Rating: daedalus.apache.org 1.6.2 0/1000/N > 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); > } > } >