Hello, This is a repost of a message sent to the Subversion development mailing list containing a patch for APR.
Windows has a bug in its WriteFile API. When the file handle is a console output handle and the data chunk is greater than some value around 60 KB, instead of doing either a complete write (which would be ideal) or a partial write (which is permitted by the API contract), it returns an incorrect error causing Subversion to display an error message such as: svn: Can't write to stream: Not enough storage is available to process this command. Clearly, as the actual bug is in Windows, we can't fix the bug itself, so we need a work-around, and common sense dictates that that work-around be as close as possible to the API. So, the patch is for APR. The obvious work-around is to take large writes and divide them up into smaller writes. APR already uses separate files for file I/O on different platforms, which means the work-around will be automatically segregated to Win32 only. Even with the work-around in place, writing a large amount of data in 48 KB blocks is not going to be significantly slower compared to the ideal single API call. So, rather than add complexity and try to figure out whether the file handle is a console file handle, I simply apply the work-around to *all* file writes done through apr_file_write on Win32. I've attached a patch which achieves this, however after an hour and a half of trying to work with the Subversion build system for Windows (which I am not familiar with), I was not able to get a working build. So, I present this patch as "carefully reviewed & probably good but untested". I was careful to preserve the semantics of the border case where *nbytes is initially 0; as before, the function still calls WriteFile() with a count of 0 once. I would like to ask someone who does have a working Win32 build environment using APR head to try applying my patch and see if it fixes the problem. There is a test case for the bug at the following URL: http://israel.logiclrd.cx/svn_bug.zip Simply extract it to its own subdirectory and then issue "svn diff" within that directory on a Windows platform. It should fail prior to the patch, and, in theory, succeed with the patch applied. If anyone would like to give me some hints on building under Windows, that would also be great. I used "gen-make.py -t vcproj" as per the instructions, but ended up with projects that do not reference their dependencies and DLLs that do not export any functions at all. Without further ado, here is the patch (inline & attached as I expect the inline copy to be mangled by my e-mail client): [[[ Fix issue #1789 by working around the underlying Windows bug. * file_io/win32/readwrite.c: Made apr_file_write divide large buffers into smaller chunks. ]]] Index: file_io/win32/readwrite.c =================================================================== --- file_io/win32/readwrite.c (revision 509010) +++ file_io/win32/readwrite.c (working copy) @@ -24,6 +24,16 @@ #include "apr_arch_atime.h" #include "apr_arch_misc.h" +/* This value is used to divide write operations into smaller + * chunks due to a bug in Windows that occurs when WriteFile + * is used on chunks larger than 64KB. Instead of doing a + * partial write (or internally looping in order to do a full + * write), it returns an error: + * + * "Not enough storage is available to process this command." + */ +#define MAXIMUM_SYNCHRONOUS_WRITE_SIZE (48 * 1024) + /* * read_with_timeout() * Uses async i/o to emulate unix non-blocking i/o with timeouts. @@ -309,9 +319,48 @@ if (thefile->pOverlapped) { thefile->pOverlapped->Offset = (DWORD)thefile->filePtr; thefile->pOverlapped->OffsetHigh = (DWORD)(thefile->filePtr >> 32); + rv = WriteFile(thefile->filehand, buf, (DWORD)*nbytes, + &bwrote, thefile->pOverlapped); } - rv = WriteFile(thefile->filehand, buf, (DWORD)*nbytes, &bwrote, - thefile->pOverlapped); + else { + /* The write may need to be divided up into chunks due to a + * bug in WriteFile when it is given a console file handle. + */ + bwrote = 0; + do { + /* Determine the next chunk's size and write it. */ + DWORD chunk_size = MAXIMUM_SYNCHRONOUS_WRITE_SIZE; + + if (chunk_size > *nbytes) { + chunk_size = *nbytes; + } + + rv = WriteFile(thefile->filehand, buf, + chunk_size, &chunk_size, NULL); + + /* If an error occurs but some bytes were written, report + * success for those bytes only. A subsequent call will + * return the same error without any bytes having been + * written. + */ + if (!rv) { + if (bwrote > 0) { + /* By making rv non-zero, the return-value test + * below will return APR_SUCCESS. + */ + rv = TRUE; + } + break; + } + + /* Tally the bytes that were written and advance the + * buffer pointer. */ + bwrote += chunk_size; + + buf = chunk_size + (char *)buf; + *nbytes -= chunk_size; + } while (*nbytes > 0); + } if (thefile->append) { apr_file_unlock(thefile); apr_thread_mutex_unlock(thefile->mutex);
apr_patch.diff.gz
Description: Binary data
