You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Jonathan Gilbert <o2...@sneakemail.com> on 2007/02/19 06:01:23 UTC
[PATCH] Fix for issue #1789
Hello,
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);
Re: [PATCH] Fix for issue #1789
Posted by "Hyrum K. Wright" <hy...@mail.utexas.edu>.
Erik Huelsmann wrote:
> On 4/3/07, Hyrum K. Wright <hy...@mail.utexas.edu> wrote:
>> Jonathan Gilbert wrote:
>> > At 12:23 AM 2/19/2007 -0600, I wrote:
>> >> At 09:13 AM 2/19/2007 +0300, Ivan Zhakov wrote:
>> >>> I had the same problem. My problem was that Visual Studio cannot find
>> > Python.
>> >>> Try add path to Python binary directory at Visual Studio binary
>> >>> directories list.
>> >> Huh? That's confusing to me, as I didn't see Visual Studio trying
>> to run
>> >> any python scripts during the build process. However, I'll give it
>> a try;
>> >> hopefully it'll clear everything up for me as well :-)
>> >
>> > Well, this fixed my build problem, and I can confirm that the bug
>> described
>> > by Subversion issue #1789 is plugged nicely by the latest revision
>> of my
>> > patch to APR. I verified that the diff output (nearly 70 KB -- by
>> > coincidence, the same size as the random test case I threw together
>> before
>> > I knew what the problem was) was complete & correct by applying it with
>> > "patch.exe" to a copy of the text-base and checking that I did in
>> fact get
>> > the identical modifications originally made to the file.
>> >
>> > If the patch is accepted by the APR team, perhaps it could be part
>> of the
>> > next release of Subversion :-)
>>
>> Jonathan,
>> Do you have an update on whether or not your patch has been committed
>> into APR? If so, does is solve the problem in issue 1789?
>
> Hi Hyrum,
>
> The fix hasn't been committed to APR, but we have a workaround fix
> committed to trunk (and it's nominated for backport to 1.4.x).
Great!
Thanks,
-Hyrum
Re: [PATCH] Fix for issue #1789
Posted by Erik Huelsmann <eh...@gmail.com>.
On 4/3/07, Hyrum K. Wright <hy...@mail.utexas.edu> wrote:
> Jonathan Gilbert wrote:
> > At 12:23 AM 2/19/2007 -0600, I wrote:
> >> At 09:13 AM 2/19/2007 +0300, Ivan Zhakov wrote:
> >>> I had the same problem. My problem was that Visual Studio cannot find
> > Python.
> >>> Try add path to Python binary directory at Visual Studio binary
> >>> directories list.
> >> Huh? That's confusing to me, as I didn't see Visual Studio trying to run
> >> any python scripts during the build process. However, I'll give it a try;
> >> hopefully it'll clear everything up for me as well :-)
> >
> > Well, this fixed my build problem, and I can confirm that the bug described
> > by Subversion issue #1789 is plugged nicely by the latest revision of my
> > patch to APR. I verified that the diff output (nearly 70 KB -- by
> > coincidence, the same size as the random test case I threw together before
> > I knew what the problem was) was complete & correct by applying it with
> > "patch.exe" to a copy of the text-base and checking that I did in fact get
> > the identical modifications originally made to the file.
> >
> > If the patch is accepted by the APR team, perhaps it could be part of the
> > next release of Subversion :-)
>
> Jonathan,
> Do you have an update on whether or not your patch has been committed
> into APR? If so, does is solve the problem in issue 1789?
Hi Hyrum,
The fix hasn't been committed to APR, but we have a workaround fix
committed to trunk (and it's nominated for backport to 1.4.x).
bye,
Erik.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] Fix for issue #1789
Posted by "Hyrum K. Wright" <hy...@mail.utexas.edu>.
Jonathan Gilbert wrote:
> At 12:23 AM 2/19/2007 -0600, I wrote:
>> At 09:13 AM 2/19/2007 +0300, Ivan Zhakov wrote:
>>> I had the same problem. My problem was that Visual Studio cannot find
> Python.
>>> Try add path to Python binary directory at Visual Studio binary
>>> directories list.
>> Huh? That's confusing to me, as I didn't see Visual Studio trying to run
>> any python scripts during the build process. However, I'll give it a try;
>> hopefully it'll clear everything up for me as well :-)
>
> Well, this fixed my build problem, and I can confirm that the bug described
> by Subversion issue #1789 is plugged nicely by the latest revision of my
> patch to APR. I verified that the diff output (nearly 70 KB -- by
> coincidence, the same size as the random test case I threw together before
> I knew what the problem was) was complete & correct by applying it with
> "patch.exe" to a copy of the text-base and checking that I did in fact get
> the identical modifications originally made to the file.
>
> If the patch is accepted by the APR team, perhaps it could be part of the
> next release of Subversion :-)
Jonathan,
Do you have an update on whether or not your patch has been committed
into APR? If so, does is solve the problem in issue 1789?
-Hyrum
Re: [PATCH] Fix for issue #1789
Posted by Jonathan Gilbert <o2...@sneakemail.com>.
At 12:23 AM 2/19/2007 -0600, I wrote:
>At 09:13 AM 2/19/2007 +0300, Ivan Zhakov wrote:
>>I had the same problem. My problem was that Visual Studio cannot find
Python.
>>Try add path to Python binary directory at Visual Studio binary
>>directories list.
>
>Huh? That's confusing to me, as I didn't see Visual Studio trying to run
>any python scripts during the build process. However, I'll give it a try;
>hopefully it'll clear everything up for me as well :-)
Well, this fixed my build problem, and I can confirm that the bug described
by Subversion issue #1789 is plugged nicely by the latest revision of my
patch to APR. I verified that the diff output (nearly 70 KB -- by
coincidence, the same size as the random test case I threw together before
I knew what the problem was) was complete & correct by applying it with
"patch.exe" to a copy of the text-base and checking that I did in fact get
the identical modifications originally made to the file.
If the patch is accepted by the APR team, perhaps it could be part of the
next release of Subversion :-)
Jonathan Gilbert
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] Fix for issue #1789
Posted by Jonathan Gilbert <2j...@sneakemail.com>.
At 12:23 AM 2/19/2007 -0600, I wrote:
>At 09:13 AM 2/19/2007 +0300, Ivan Zhakov wrote:
>>I had the same problem. My problem was that Visual Studio cannot find
Python.
>>Try add path to Python binary directory at Visual Studio binary
>>directories list.
>
>Huh? That's confusing to me, as I didn't see Visual Studio trying to run
>any python scripts during the build process. However, I'll give it a try;
>hopefully it'll clear everything up for me as well :-)
Well, this fixed my build problem, and I can confirm that the bug described
by Subversion issue #1789 is plugged nicely by the latest revision of my
patch to APR. I verified that the diff output (nearly 70 KB -- by
coincidence, the same size as the random test case I threw together before
I knew what the problem was) was complete & correct by applying it with
"patch.exe" to a copy of the text-base and checking that I did in fact get
the identical modifications originally made to the file.
If the patch is accepted by the APR team, perhaps it could be part of the
next release of Subversion :-)
Jonathan Gilbert
Re: [PATCH] Fix for issue #1789
Posted by Jonathan Gilbert <o2...@sneakemail.com>.
At 09:13 AM 2/19/2007 +0300, Ivan Zhakov wrote:
>On 2/19/07, Jonathan Gilbert <o2...@sneakemail.com> wrote:
>> 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.
>
>APR is separate project with his own mailing list:
>dev@apr.apache.org
>
>I think this patch will be appropriate for that mailing list. Also did
>you check trunk version of APR? Probably this bug fixed.
People on irc.freenode.net #svn-dev suggested that as many Subversion
committers are also APR committers, it would be appropriate to send the
patch to this list. The patch was made against head of APR trunk, so I'm
fairly certain the issue had not already been addressed. :-)
I will send the patch to dev@apr.apache.org as well.
>> 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.
>I had the same problem. My problem was that Visual Studio cannot find Python.
>Try add path to Python binary directory at Visual Studio binary
>directories list.
Huh? That's confusing to me, as I didn't see Visual Studio trying to run
any python scripts during the build process. However, I'll give it a try;
hopefully it'll clear everything up for me as well :-)
Thanks,
Jonathan Gilbert
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] Fix for issue #1789
Posted by Ivan Zhakov <ch...@gmail.com>.
On 2/19/07, Jonathan Gilbert <o2...@sneakemail.com> wrote:
> Hello,
>
> 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.
APR is separate project with his own mailing list:
dev@apr.apache.org
I think this patch will be appropriate for that mailing list. Also did
you check trunk version of APR? Probably this bug fixed.
>
> 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.
I had the same problem. My problem was that Visual Studio cannot find Python.
Try add path to Python binary directory at Visual Studio binary
directories list.
--
Ivan Zhakov
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org