I've refactored the read_with_timeout() code to account for observations
about both WAIT_ABANDONED and the short race between ReadFile, WaitFor...
CancelIo and GetCompletion.
It passes regressions which isn't saying much, since a number of edge cases
in the code are fixed which never appeared in the regression testall framework.
I need 1) feedback, and 2) opinions...
[ ] code's ready to be adopted as our 'write_with_timeout' logic
[ ] code should be backported to 1.2.x now
[ ] code should be backported to 0.9.x now
[ ] code should be rolled out in the 0.9.11 release already
Bill
--- Begin Message ---
Author: wrowe
Date: Thu Mar 23 13:49:49 2006
New Revision: 388281
URL: http://svn.apache.org/viewcvs?rev=388281&view=rev
Log:
Part 1 of many read_with_timeout logic fixes. Stop polluting one
occurance of rv with the boolean result of ReadFile() to increase
the legibility of the success/failure of ReadFile. This requires
us to defer *nbytes assignment to the function's end.
This fix catches additional cases of APR_EOF, as we had not
tested this case from the error handling path. So any deferred
read of zero bytes previously returned 0 bytes APR_SUCCESS
rather than APR_EOF. (This occurs when we wait to discover the
owner of the write end closes it without additional data)
Modified:
apr/apr/trunk/file_io/win32/readwrite.c
Modified: apr/apr/trunk/file_io/win32/readwrite.c
URL:
http://svn.apache.org/viewcvs/apr/apr/trunk/file_io/win32/readwrite.c?rev=388281&r1=388280&r2=388281&view=diff
==============================================================================
--- apr/apr/trunk/file_io/win32/readwrite.c (original)
+++ apr/apr/trunk/file_io/win32/readwrite.c Thu Mar 23 13:49:49 2006
@@ -72,11 +72,11 @@
file->pOverlapped->OffsetHigh = (DWORD)(file->filePtr >> 32);
}
- rv = ReadFile(file->filehand, buf, len,
- &bytesread, file->pOverlapped);
- *nbytes = bytesread;
-
- if (!rv) {
+ if (ReadFile(file->filehand, buf, len,
+ &bytesread, file->pOverlapped)) {
+ rv = APR_SUCCESS;
+ }
+ else {
rv = apr_get_os_error();
if (rv == APR_FROM_OS_ERROR(ERROR_IO_PENDING)) {
/* Wait for the pending i/o */
@@ -117,16 +117,16 @@
/* Assume ERROR_BROKEN_PIPE signals an EOF reading from a pipe */
rv = APR_EOF;
}
- } else {
- /* OK and 0 bytes read ==> end of file */
- if (*nbytes == 0)
- rv = APR_EOF;
- else
- rv = APR_SUCCESS;
+ }
+
+ /* OK and 0 bytes read ==> end of file */
+ if (rv == APR_SUCCESS && bytesread == 0)
+ rv = APR_EOF;
}
if (rv == APR_SUCCESS && file->pOverlapped && !file->pipe) {
- file->filePtr += *nbytes;
+ file->filePtr += bytesread;
}
+ *nbytes = bytesread;
return rv;
}
--- End Message ---
--- Begin Message ---
Author: wrowe
Date: Thu Mar 23 13:51:11 2006
New Revision: 388282
URL: http://svn.apache.org/viewcvs?rev=388282&view=rev
Log:
Part 2 of the necessary read_with_timeout() fixes. Catch the
condition where our broken pipe occurs durring the deferred
i/o completion phase.
Modified:
apr/apr/trunk/file_io/win32/readwrite.c
Modified: apr/apr/trunk/file_io/win32/readwrite.c
URL:
http://svn.apache.org/viewcvs/apr/apr/trunk/file_io/win32/readwrite.c?rev=388282&r1=388281&r2=388282&view=diff
==============================================================================
--- apr/apr/trunk/file_io/win32/readwrite.c (original)
+++ apr/apr/trunk/file_io/win32/readwrite.c Thu Mar 23 13:51:11 2006
@@ -113,7 +113,7 @@
}
}
}
- else if (rv == APR_FROM_OS_ERROR(ERROR_BROKEN_PIPE)) {
+ if (rv == APR_FROM_OS_ERROR(ERROR_BROKEN_PIPE)) {
/* Assume ERROR_BROKEN_PIPE signals an EOF reading from a pipe */
rv = APR_EOF;
}
.
--- End Message ---
--- Begin Message ---
Author: wrowe
Date: Thu Mar 23 14:44:16 2006
New Revision: 388285
URL: http://svn.apache.org/viewcvs?rev=388285&view=rev
Log:
Minor typo fix to previous commit.
Modified:
apr/apr/trunk/file_io/win32/readwrite.c
Modified: apr/apr/trunk/file_io/win32/readwrite.c
URL:
http://svn.apache.org/viewcvs/apr/apr/trunk/file_io/win32/readwrite.c?rev=388285&r1=388284&r2=388285&view=diff
==============================================================================
--- apr/apr/trunk/file_io/win32/readwrite.c (original)
+++ apr/apr/trunk/file_io/win32/readwrite.c Thu Mar 23 14:44:16 2006
@@ -122,7 +122,7 @@
/* OK and 0 bytes read ==> end of file */
if (rv == APR_SUCCESS && bytesread == 0)
rv = APR_EOF;
- }
+
if (rv == APR_SUCCESS && file->pOverlapped && !file->pipe) {
file->filePtr += bytesread;
}
.
--- End Message ---
--- Begin Message ---
Author: wrowe
Date: Thu Mar 23 15:19:32 2006
New Revision: 388292
URL: http://svn.apache.org/viewcvs?rev=388292&view=rev
Log:
Part three of the read_with_timeout refactoring.
Loop on the WaitForSingleObject if it indicated WAIT_ABANDONED,
which occurs when the thread/proc which created the event exits,
and ownership of the event has been transfered.
Always try to CancelIo if the wait has failed.
Ignore the Wait/Cancel results and then always check the completion
status of the original Read. Indicate TIMEUP when appropriate.
Modified:
apr/apr/trunk/file_io/win32/readwrite.c
Modified: apr/apr/trunk/file_io/win32/readwrite.c
URL:
http://svn.apache.org/viewcvs/apr/apr/trunk/file_io/win32/readwrite.c?rev=388292&r1=388291&r2=388292&view=diff
==============================================================================
--- apr/apr/trunk/file_io/win32/readwrite.c (original)
+++ apr/apr/trunk/file_io/win32/readwrite.c Thu Mar 23 15:19:32 2006
@@ -31,6 +31,7 @@
static apr_status_t read_with_timeout(apr_file_t *file, void *buf, apr_size_t
len_in, apr_size_t *nbytes)
{
apr_status_t rv;
+ DWORD res;
DWORD len = (DWORD)len_in;
DWORD bytesread = 0;
@@ -79,38 +80,43 @@
else {
rv = apr_get_os_error();
if (rv == APR_FROM_OS_ERROR(ERROR_IO_PENDING)) {
- /* Wait for the pending i/o */
- if (file->timeout > 0) {
- /* timeout in milliseconds... */
- rv = WaitForSingleObject(file->pOverlapped->hEvent,
- (DWORD)(file->timeout/1000));
- }
- else if (file->timeout == -1) {
- rv = WaitForSingleObject(file->pOverlapped->hEvent, INFINITE);
+ /* Wait for the pending i/o, timeout converted from us to ms
+ * Note that we loop if someone gives up the event, since
+ * folks suggest that WAIT_ABANDONED isn't actually a result
+ * but an alert that ownership of the event has passed from
+ * one owner to a new proc/thread.
+ */
+ do {
+ res = WaitForSingleObject(file->pOverlapped->hEvent,
+ (file->timeout > 0)
+ ? (DWORD)(file->timeout/1000)
+ : ((file->timeout == -1)
+ ? INFINITE : 0));
+ } while (res == WAIT_ABANDONED);
+
+ /* There is one case that represents entirely
+ * successful operations, otherwise we will cancel
+ * the operation in progress.
+ */
+ if (res != WAIT_OBJECT_0) {
+ CancelIo(file->filehand);
}
- switch (rv) {
- case WAIT_OBJECT_0:
- GetOverlappedResult(file->filehand, file->pOverlapped,
- &bytesread, TRUE);
- rv = APR_SUCCESS;
- break;
-
- case WAIT_TIMEOUT:
- rv = APR_TIMEUP;
- break;
-
- case WAIT_FAILED:
- rv = apr_get_os_error();
- break;
- default:
- break;
+ /* Ignore any failures above. Attempt to complete
+ * the overlapped operation and use only _its_ result.
+ * For example, CancelIo or WaitForSingleObject can
+ * fail if the handle is closed, yet the read may have
+ * completed before we attempted to CancelIo...
+ */
+ if (GetOverlappedResult(file->filehand, file->pOverlapped,
+ &bytesread, TRUE)) {
+ rv = APR_SUCCESS;
}
-
- if (rv != APR_SUCCESS) {
- if (apr_os_level >= APR_WIN_98) {
- CancelIo(file->filehand);
- }
+ else {
+ rv = apr_get_os_error();
+ if (rv == APR_FROM_OS_ERROR(ERROR_IO_INCOMPLETE)
+ && res == WAIT_TIMEOUT)
+ rv = APR_TIMEUP;
}
}
if (rv == APR_FROM_OS_ERROR(ERROR_BROKEN_PIPE)) {
--- End Message ---
