Re: [VOTE] Release apr-1.7.4-rc1 as apr-1.7.4
Eric Covener writes: > Hi all, > > Please find below the proposed release tarball and signatures: > > https://dist.apache.org/repos/dist/dev/apr/ > > I would like to call a VOTE over the next few days to release > this candidate tarball apr-1.7.4-rc1 as 1.7.4: [X] +1: It's not just good, it's good enough! Checks performed: — Verify SHA256 for apr-1.7.4-rc1-candidate.tar.bz2 — Verify SHA256 for apr-1.7.4-rc1-candidate.tar.gz — Verify SHA256 for apr-1.7.4-rc1-candidate-win32-src.zip — Verify diff between apr-1.7.4-rc1-candidate-win32-src.zip and published apr-1.7.3-win32-src.zip — Build using apr-1.7.4-rc1-candidate-win32-src.zip on Windows x64/CMake — Run APR tests on Windows x64 — Run Subversion 1.14.x tests with this APR version on Windows x64 Thanks, Evgeny Kotkov
Re: [VOTE] Release apr-1.7.4-rc1 as apr-1.7.4
Eric Covener writes: > Hi all, > > Please find below the proposed release tarball and signatures: > > https://dist.apache.org/repos/dist/dev/apr/ First time voting for an APR release, but isn't apr-1.7.4-rc1-win32-src.zip missing from the set of artifacts? I think that it was present during the vote for APR 1.7.3: [[[ > svn log https://dist.apache.org/repos/dist/dev/apr -c60838 -v Changed paths: M /dev/apr/Announcement1.x.html M /dev/apr/Announcement1.x.txt A /dev/apr/apr-1.7.3-rc1-win32-src.zip A /dev/apr/apr-1.7.3-rc1-win32-src.zip.asc A /dev/apr/apr-1.7.3-rc1-win32-src.zip.sha256 A /dev/apr/apr-1.7.3-rc1.tar.bz2 A /dev/apr/apr-1.7.3-rc1.tar.bz2.asc A /dev/apr/apr-1.7.3-rc1.tar.bz2.sha256 A /dev/apr/apr-1.7.3-rc1.tar.gz A /dev/apr/apr-1.7.3-rc1.tar.gz.asc A /dev/apr/apr-1.7.3-rc1.tar.gz.sha256 1.7.3. rc1 ]]] Thanks, Evgeny Kotkov
Re: Release APR 1.7.4?
Yann Ylavic writes: > Do we want to include r1909117 in 1.7.4 (to call abort() instead of > assert() in apr_base64 when NDEBUG)? > Sorry for the lateness, but should there be an -rc2 (or in any case..). If I recall correctly, APR 1.7.x doesn't have apr_base64, because it is a part of APR-util in that timeline. Thanks, Evgeny Kotkov
Re: Release APR 1.7.4?
Eric Covener writes: > I will RM but it might be a bit piecemeal between now and the weekend. > At the mercy of a lot of inflexible $bigco stuff. > > Hoping to improve a little this time by porting more of icings amazing > httpd release work. Awesome, thanks! Regards, Evgeny Kotkov
Re: svn commit: r1909094 - in /apr/apr/branches/1.8.x: ./ CHANGES
Christophe JAILLET writes: > +1 for me. > > Looking at 1.7 and 1.6 it seems it was done this way in previous releases. Committed in https://svn.apache.org/r1909103 Thanks, Evgeny Kotkov
Re: svn commit: r1909094 - in /apr/apr/branches/1.8.x: ./ CHANGES
Christophe JAILLET writes: > If we remove things that are part of previous 1.7.x releases, shouldn't > Changes for APR 1.7.[23] be added here instead? Hm, since these are the 1.8.x CHANGES maybe it'd be better to drop the 1.7.1, 1.7.2 and 1.7.3 entries from here and just link to branches/1.7.x/CHANGES, to avoid maintaining two separate copies of 1.7.x CHANGES. How does that sound? Thanks, Evgeny Kotkov
Release APR 1.7.4?
Hi everyone, Unfortunately, one of the fixes in APR 1.7.3 that was authored by me has caused a significant regression, where writing to files opened with both APR_FOPEN_APPEND and APR_FOPEN_BUFFERED now may not properly append the data on Windows. The issue has been reported in [1]. Compared to the potential impact of the regression, the original fix seems to be of lesser concern, so I reverted the change in r1909088 [2] and backported it to 1.7.x in r1909095 [3]. Would it be feasible to release 1.7.4 with a fix for this regression? (I can try to RM if needed, although in such a case it would be my first time preparing an APR release.) [1] https://lists.apache.org/thread/56gnyc3tc0orjh5mfsqo9gpq1br59b01 [2] https://svn.apache.org/r1909088 [3] https://svn.apache.org/r1909095 Thanks, Evgeny Kotkov
Re: svn commit: r1902206 - /apr/apr/trunk/encoding/apr_base64.c
Yann Ylavic writes:
> So possibly something like the below:
>
> Index: encoding/apr_base64.c
> ===
> --- encoding/apr_base64.c(revision 1908764)
> +++ encoding/apr_base64.c(working copy)
> @@ -20,8 +20,27 @@
> * ugly 'len' functions, which is quite a nasty cost.
> */
>
> -#undef NDEBUG /* always abort() on assert()ion failure */
> +/* An APR__ASSERT()ion failure will abort() with or without printing
> + * the faulting condition (and code location) depending on NDEBUG.
> + */
> +#ifndef NDEBUG /* use assert()/system's printing before abort() mechanism */
> #include
> +#define APR__ASSERT(cond) assert(cond)
> +#else /* just abort() */
> +#if APR_HAVE_STDLIB_H
> +#include
> +#endif
> +#if HAVE___BUILTIN_EXPECT
> +#define APR__UNLIKELY(cond) __builtin_expect(!!(cond), 0)
> +#else
> +#define APR__UNLIKELY(cond) (!!(cond))
> +#endif
> +#define APR__ASSERT(cond) do { \
> +if (APR__UNLIKELY(!(cond))) { \
> +abort(); \
> +} \
> +} while (0)
> +#endif
Yes, something along these lines, although I was thinking we could maybe
get away with just adding a small helper that covers all cases, as in the
attached patch.
Thanks,
Evgeny Kotkov
Index: encoding/apr_base64.c
===
--- encoding/apr_base64.c (revision 1908804)
+++ encoding/apr_base64.c (working copy)
@@ -20,7 +20,6 @@
* ugly 'len' functions, which is quite a nasty cost.
*/
-#undef NDEBUG /* always abort() on assert()ion failure */
#include
#include "apr_base64.h"
@@ -28,6 +27,17 @@
#include "apr_xlate.h"
#endif/* APR_CHARSET_EBCDIC */
+#if APR_HAVE_STDLIB_H
+#include
+#endif
+
+#define verify_condition(cond) do { \
+if (!(cond)) { \
+assert(0 && #cond); \
+abort();\
+} \
+} while (0)
+
/* Above APR_BASE64_ENCODE_MAX length the encoding can't fit in an int >= 0 */
#define APR_BASE64_ENCODE_MAX 1610612733
@@ -124,7 +134,7 @@ APR_DECLARE(int) apr_base64_decode_len(const char
bufin = (const unsigned char *) bufcoded;
while (pr2six[*(bufin++)] <= 63);
nprbytes = (bufin - (const unsigned char *) bufcoded) - 1;
-assert(nprbytes <= APR_BASE64_DECODE_MAX);
+verify_condition(nprbytes <= APR_BASE64_DECODE_MAX);
return (int)(((nprbytes + 3u) / 4u) * 3u + 1u);
}
@@ -161,7 +171,7 @@ APR_DECLARE(int) apr_base64_decode_binary(unsigned
bufin = (const unsigned char *) bufcoded;
while (pr2six[*(bufin++)] <= 63);
nprbytes = (bufin - (const unsigned char *) bufcoded) - 1;
-assert(nprbytes <= APR_BASE64_DECODE_MAX);
+verify_condition(nprbytes <= APR_BASE64_DECODE_MAX);
nbytesdecoded = (int)(((nprbytes + 3u) / 4u) * 3u);
bufout = (unsigned char *) bufplain;
@@ -206,7 +216,7 @@ static const char basis_64[] =
APR_DECLARE(int) apr_base64_encode_len(int len)
{
-assert(len >= 0 && len <= APR_BASE64_ENCODE_MAX);
+verify_condition(len >= 0 && len <= APR_BASE64_ENCODE_MAX);
return ((len + 2) / 3 * 4) + 1;
}
@@ -219,7 +229,7 @@ APR_DECLARE(int) apr_base64_encode(char *encoded,
int i;
char *p;
-assert(len >= 0 && len <= APR_BASE64_ENCODE_MAX);
+verify_condition(len >= 0 && len <= APR_BASE64_ENCODE_MAX);
p = encoded;
for (i = 0; i < len - 2; i += 3) {
@@ -258,7 +268,7 @@ APR_DECLARE(int) apr_base64_encode_binary(char *en
int i;
char *p;
-assert(len >= 0 && len <= APR_BASE64_ENCODE_MAX);
+verify_condition(len >= 0 && len <= APR_BASE64_ENCODE_MAX);
p = encoded;
for (i = 0; i < len - 2; i += 3) {
@@ -292,7 +302,7 @@ APR_DECLARE(char *) apr_pbase64_encode(apr_pool_t
char *encoded;
apr_size_t len = strlen(string);
-assert(len <= (apr_size_t)APR_BASE64_ENCODE_MAX);
+verify_condition(len <= (apr_size_t)APR_BASE64_ENCODE_MAX);
encoded = (char *) apr_palloc(p, apr_base64_encode_len((int)len));
apr_base64_encode(encoded, string, (int)len);
Re: svn commit: r1902206 - /apr/apr/trunk/encoding/apr_base64.c
Yann Ylavic writes: > +#undef NDEBUG /* always abort() on assert()ion failure */ > +#include Sorry for being late to the party, but I think there are a few problems with the "#undef NDEBUG" line: 1) The debug implementation of an assert() may print a diagnostic message, for example to stderr. A caller of the library function may not be ready for this to happen when using a non-debug version of the library. 2) The actual destination of the message seems to be implementation-defined. For example, in Windows-based applications this may show a message box [1], which is probably even more unexpected for the user of the library. 3) Undefining NDEBUG before other headers may silently cause unexpected effects if any of those headers make some decisions based on the NDEBUG value, which isn't an entirely unreasonable thing to expect. So, depending on whether we want the checks to soft- or hard-fault, maybe we could either remove the "#undef NDEBUG" line, or switch to a custom check that explicitly calls an abort()? [1] https://learn.microsoft.com/en-us/cpp/c-runtime-library/reference/assert-macro-assert-wassert#remarks Thanks, Evgeny Kotkov
Re: svn commit: r1902375 - in /apr/apr/branches/1.8.x: ./ CHANGES file_io/win32/readwrite.c test/testfile.c
Yann Ylavic writes: > Oh, and the loop seems to have been removed later in r1828509 > "apr_file_read: Optimize large reads from buffered files on Windows" > (backported to 2.4.x in r1902379 too). > So Windows is doing the right thing now (not unix), right? Yes, AFAICT unix still uses the original approach. > Sorry for the noise, and thanks for the fix/info :) Thanks, Evgeny Kotkov
Re: svn commit: r1902378 - in /apr/apr/branches/1.8.x: ./ CHANGES file_io/win32/readwrite.c test/testfile.c
Yann Ylavic writes: > But the short-write argument stands, if Windows never short-writes > then the implicit full-write is unnecessary, but if it does > short-write then apr_file_write() has to return it too (for the user > to know), so in both cases the loop has to stop when written < > to_write (and probably return success if written > 0). > Users that don't expect short-writes should use apr_file_write_full(). As far as I recall, I had an intent of not changing the behavior more than necessary, as the original code was always making a full write. I agree that we could potentially allow for short writes when performing the passthrough part of the write that makes its way around the buffer. But all in all, I think that we might want to keep the current approach, because: — This won't change the actual behavior for Win32 files from a practical standpoint, because synchronous WriteFile() guarantees a full write for them. — There's a tricky case where we need to handle a large write with len > DWORD_MAX. I think that we might want to not handle it with a short write, because if we do so, we're going to have an actually reproducible short write for files (i.e., different behavior) in the rare edge case. — Flushing the buffer requires a full write, so at least a part of the buffered apr_file_write() is still going to do a full write. Regards, Evgeny Kotkov
Re: svn commit: r1902375 - in /apr/apr/branches/1.8.x: ./ CHANGES file_io/win32/readwrite.c test/testfile.c
Yann Ylavic writes:
> Why would buffered reads always be full reads (i.e. until the given
> size of EOF)?
>
> For apr_file_read() I don't think this is expected, users might want
> read() to return what is there when it's there (i.e. they handle short
> reads by themselves), so implicit full reads will force them to use
> small buffers artificially and they'll lose in throughput. On the
> other hand, users that want full reads can do that already/currently
> in their own code (by calling read() multiple times).
>
> For apr_file_gets() this is the same issue, why when some read returns
> the \n would we need to continue reading to fill the buffer? In a pipe
> scenario the peer would apr_file_write("Hello\n") and the consumer's
> apr_file_read() would not return that to the user immediately?
>
> This has nothing to do with buffered vs non-buffered IMHO, there are
> apr_file_{read,write}_full() already for that.
One comment I have here is that this commit does not change the existing
behavior of apr_file_read() for buffered files. The read_buffered() helper
isn't new by itself, as it has been merely factored out from apr_file_read().
So, doing full reads for buffered files has been the current behavior
for both unix and win32 and for a long time, I think. For example, see
unix/readwrite.c:file_read_buffered(). This applies to the unix version
of apr_file_gets() as well.
Regards,
Evgeny Kotkov
Re: svn commit: r1896510 - in /apr/apr/branches/1.7.x: ./ file_io/win32/ include/arch/unix/ include/arch/win32/ network_io/os2/ poll/os2/ poll/unix/
Ivan Zhakov writes: > This part is now in the following branch: > https://svn.ostyserver.net/svn/asf/apr/apr/branches/win32-pollset-wakeup-no-file-socket-emulation > > What do you think? > > It would be great if someone could take a look on the implementation from > the *nix perspective. > After that, I propose to merge the branch into trunk. In case this helps, I have tested the branch on Ubuntu x64 and it seems to compile and pass the tests. (While here, I noticed that APR trunk doesn't build on Windows; have posted on that separately.) Thanks, Evgeny Kotkov
Re: svn commit: r1897207 - in /apr/apr/trunk: include/apr_thread_proc.h threadproc/beos/thread.c threadproc/netware/thread.c threadproc/os2/thread.c threadproc/unix/thread.c threadproc/win32/thread.c
Yann Ylavic writes:
> +APR_DECLARE(apr_status_t) apr_thread_current_create(apr_thread_t **current,
> +apr_threadattr_t *attr,
> +apr_pool_t *pool)
> +{
> +apr_status_t stat;
> +
> +*current = apr_thread_current();
> +if (*current) {
> +return APR_EEXIST;
> +}
> +
> +stat = alloc_thread(current, attr, NULL, NULL, pool);
> +if (stat != APR_SUCCESS) {
> +return stat;
> +}
> +
> +if (!(attr && attr->detach)) {
> +(*new)->td = apr_os_thread_current();
> +}
Hi Yann,
It looks like this hunk doesn't compile on Windows:
…\threadproc\win32\thread.c(193): error C2065: 'new': undeclared identifier
…\threadproc\win32\thread.c(193): error C2100: illegal indirection
…\threadproc\win32\thread.c(193): error C2223: left of '->td' must point
to struct/union
Thanks,
Evgeny Kotkov
Re: svn commit: r1833786 - /apr/apr/branches/1.6.x/STATUS
William A Rowe Jr writes: > Confusing... which patch is missing? > > http://svn.apache.org/viewvc?view=revision=1808457 > http://svn.apache.org/viewvc?view=revision=1829962 > > It should not be possible at this point to need 1.x specific code > that wouldn't correspond to a trunk commit. I think that r1829962 should be treated as a follow-up to r1808457 (where I missed one calling site of apr_thread_mutex_unlock() in buffer.c). In other words, they should either be backported together, or not backported at all. Since this change is a relatively minor optimization (not a fix), I would think that it might make sense to keep it just in trunk. Thanks, Evgeny Kotkov
Re: svn commit: r1828509 - in /apr/apr/trunk: CHANGES file_io/win32/readwrite.c test/testfile.c
William A Rowe Jr <wr...@rowe-clan.net> writes: > Awesome! > > This looks entirely appropriate for 1.7 and even a 1.6.4 release, > since the changes are all "under the hood". Thanks! I will try to backport this change sometime later this week. Regards, Evgeny Kotkov
Re: [PATCH] Create and use file mutex only for files opened with APR_FOPEN_XTHREAD
Evgeny Kotkov <evgeny.kot...@visualsvn.com> writes: > Currently, there are some cases in the Windows implementation of the file > I/O that cause the unnecessary creation and acquisitions of the file mutex. > These cases include handling O_APPEND-style writes and the implementation > of the apr_file_buffer_set(). > > Creating and acquiring the file mutex is only required for files opened > with the APR_FOPEN_XTHREAD flag. Otherwise, concurrent operations > on the same apr_file_t instance should be serialized by the user, and > the mutex is not required. > > This patch tweaks the implementation to only create and acquire/release > the mutex for files opened with APR_FOPEN_XTHREAD. The log message > is included in the beginning of the patch file. Committed in https://svn.apache.org/r1808457 Regards, Evgeny Kotkov
Re: [PATCH] Don't seek to the end when opening files with APR_FOPEN_APPEND
Evgeny Kotkov <evgeny.kot...@visualsvn.com> writes: > Currently, the Windows implementation of APR_FOPEN_APPEND flag performs > a seek to the file's end when opening it. > > Apparently, this is a leftover from the very first version of handling file > appends (https://svn.apache.org/r59449) that performed a single seek to > the file's end when opening it and did not support proper atomic appends > with multiple process or threads writing to the same file. > > Since then, such atomic appends have been implemented, but the seek > was not removed. It can cause unexpected behavior when reading from > a file opened with APR_FOPEN_APPEND, assuming no writes happened > to the file. In this case, as there have been no writes, the file offset > should not be repositioned and reading should start from the beginning of > the file. However, due to the unwanted seek during open, the actual reading > would instead start from the file's end and cause an unexpected EOF. > > The attached patch fixes the issue and adds a test for it. The log message > is included in the beginning of the patch file. Committed in https://svn.apache.org/r1808456 Regards, Evgeny Kotkov
Re: svn commit: r1806603 - /apr/apr/trunk/file_io/win32/readwrite.c
Bert Huijben <b...@qqmail.nl> writes:
>> rv = WriteFile(thefile->filehand, buf, (DWORD)*nbytes, ,
>> thefile->pOverlapped);
>> +if (rv == APR_SUCCESS && thefile->pOverlapped) {
>> +thefile->filePtr += *nbytes;
>> +}
>
> The result of WriteFile should not be checked against APR_SUCCESS, as that
> is not a documented Windows result code. The right constant evaluating to
> 0 should be used.
Unfortunately, I think that I have missed how the return value of WriteFile()
was being checked in the last part of apr_file_write(), and both the r1806592
and r1806603 changesets are wrong and change the behavior of the code.
As these changesets were meant to be refactorings that make the code
clearer, without altering its behavior, I think that they should be reverted
for now.
(I will prepare an alternative patch with these simplifications that doesn't
change the behavior of the code.)
Sorry for that,
Evgeny Kotkov
[PATCH] Don't seek to the end when opening files with APR_FOPEN_APPEND
Hi everyone,
Currently, the Windows implementation of APR_FOPEN_APPEND flag performs
a seek to the file's end when opening it.
Apparently, this is a leftover from the very first version of handling file
appends (https://svn.apache.org/r59449) that performed a single seek to
the file's end when opening it and did not support proper atomic appends
with multiple process or threads writing to the same file.
Since then, such atomic appends have been implemented, but the seek
was not removed. It can cause unexpected behavior when reading from
a file opened with APR_FOPEN_APPEND, assuming no writes happened
to the file. In this case, as there have been no writes, the file offset
should not be repositioned and reading should start from the beginning of
the file. However, due to the unwanted seek during open, the actual reading
would instead start from the file's end and cause an unexpected EOF.
The attached patch fixes the issue and adds a test for it. The log message
is included in the beginning of the patch file.
Thanks,
Evgeny Kotkov
Win32: Don't seek to the end when opening files with APR_FOPEN_APPEND.
Apparently, this is a leftover from the very first version of handling file
appends (https://svn.apache.org/r59449) that performed a single seek to
the file's end when opening it and did not support proper atomic appends
with multiple process or threads writing to the same file.
Since then, such atomic appends have been implemented, but the seek
was not removed. It can cause unexpected behavior when reading from
a file opened with APR_FOPEN_APPEND, assuming no writes happened
to the file. In this case, as there have been no writes, the file offset
should not be repositioned and reading should start from the beginning of
the file. However, due to the unwanted seek during open, the actual reading
would instead start from the file's end and cause an unexpected EOF.
* file_io/win32/open.c
(apr_file_open): Don't seek to the file's end when the file is
opened with APR_FOPEN_APPEND.
* test/testfile.c
(test_append_read): New test.
(testfile): Run the new test.
Patch by: Evgeny Kotkov
Index: file_io/win32/open.c
===
--- file_io/win32/open.c(revision 1806645)
+++ file_io/win32/open.c(working copy)
@@ -445,7 +445,6 @@ APR_DECLARE(apr_status_t) apr_file_open(apr_file_t
if (flag & APR_FOPEN_APPEND) {
(*new)->append = 1;
-SetFilePointer((*new)->filehand, 0, NULL, FILE_END);
}
if (flag & APR_FOPEN_BUFFERED) {
(*new)->buffered = 1;
Index: test/testfile.c
===
--- test/testfile.c (revision 1806645)
+++ test/testfile.c (working copy)
@@ -1793,6 +1793,56 @@ static void test_append_locked(abts_case *tc, void
apr_file_remove(fname, p);
}
+static void test_append_read(abts_case *tc, void *data)
+{
+apr_status_t rv;
+apr_file_t *f;
+const char *fname = "data/testappend_read.dat";
+apr_off_t offset;
+char buf[64];
+
+apr_file_remove(fname, p);
+
+/* Create file with contents. */
+rv = apr_file_open(, fname, APR_FOPEN_WRITE | APR_FOPEN_CREATE,
+ APR_FPROT_OS_DEFAULT, p);
+APR_ASSERT_SUCCESS(tc, "create file", rv);
+
+rv = apr_file_puts("abc", f);
+APR_ASSERT_SUCCESS(tc, "write to file", rv);
+apr_file_close(f);
+
+/* Re-open it with APR_FOPEN_APPEND. */
+rv = apr_file_open(, fname,
+ APR_FOPEN_READ | APR_FOPEN_WRITE | APR_FOPEN_APPEND,
+ APR_FPROT_OS_DEFAULT, p);
+APR_ASSERT_SUCCESS(tc, "open file", rv);
+
+/* Test the initial file offset. Even though we used APR_FOPEN_APPEND,
+ * the offset should be kept in the beginning of the file until the
+ * first append. (Previously, the Windows implementation performed
+ * an erroneous seek to the file's end right after opening it.)
+ */
+offset = 0;
+rv = apr_file_seek(f, APR_CUR, );
+APR_ASSERT_SUCCESS(tc, "get file offset", rv);
+ABTS_INT_EQUAL(tc, 0, (int)offset);
+
+/* Test reading from the file. */
+rv = apr_file_gets(buf, sizeof(buf), f);
+APR_ASSERT_SUCCESS(tc, "read from file", rv);
+ABTS_STR_EQUAL(tc, "abc", buf);
+
+/* Test the file offset after reading. */
+offset = 0;
+rv = apr_file_seek(f, APR_CUR, );
+APR_ASSERT_SUCCESS(tc, "get file offset", rv);
+ABTS_INT_EQUAL(tc, 3, (int)offset);
+
+apr_file_close(f);
+apr_file_remove(fname, p);
+}
+
abts_suite *testfile(abts_suite *suite)
{
suite = ADD_SUITE(suite)
@@ -1848,6 +1898,7 @@ abts_suite *testfile(abts_suite *suite)
abts_run_test(suite, test_write_buffered_spanning_over_bufsize, NULL);
abts_run_test(suite, test_atomic_append, NULL);
abts_run_test(suite, test_append_locked, NULL);
+abts_run_test(suite, test_append_read, NULL);
return suite;
}
[PATCH] Create and use file mutex only for files opened with APR_FOPEN_XTHREAD
Hi everyone,
Currently, there are some cases in the Windows implementation of the file
I/O that cause the unnecessary creation and acquisitions of the file mutex.
These cases include handling O_APPEND-style writes and the implementation
of the apr_file_buffer_set().
Creating and acquiring the file mutex is only required for files opened
with the APR_FOPEN_XTHREAD flag. Otherwise, concurrent operations
on the same apr_file_t instance should be serialized by the user, and
the mutex is not required.
This patch tweaks the implementation to only create and acquire/release
the mutex for files opened with APR_FOPEN_XTHREAD. The log message
is included in the beginning of the patch file.
Thanks,
Evgeny Kotkov
Win32: Create and use file mutex only for files opened with APR_FOPEN_XTHREAD.
There are some cases in the Windows implementation of the file I/O that
cause the unnecessary creation and acquisitions of the file mutex.
These cases include handling O_APPEND-style writes and the implementation
of the apr_file_buffer_set().
Creating and acquiring the file mutex is only required for files opened
with the APR_FOPEN_XTHREAD flag. Otherwise, concurrent operations on
the same apr_file_t instance should be serialized by the user, and
the mutex is not required.
This patch tweaks the implementation to only create and acquire/release
the mutex for files opened with APR_FOPEN_XTHREAD.
* file_io/win32/open.c
(apr_file_open, apr_os_file_put): Create the file mutex only
with APR_FOPEN_XTHREAD.
* file_io/win32/buffer.c
(apr_file_buffer_set): Use the file mutex only with APR_FOPEN_XTHREAD.
* file_io/win32/readwrite.c
(apr_file_write): Use the file mutex only with APR_FOPEN_XTHREAD.
Patch by: Evgeny Kotkov
Index: file_io/win32/buffer.c
===
--- file_io/win32/buffer.c (revision 1806645)
+++ file_io/win32/buffer.c (working copy)
@@ -23,7 +23,9 @@ APR_DECLARE(apr_status_t) apr_file_buffer_set(apr_
{
apr_status_t rv;
-apr_thread_mutex_lock(file->mutex);
+if (file->flags & APR_FOPEN_XTHREAD) {
+apr_thread_mutex_lock(file->mutex);
+}
if(file->buffered) {
/* Flush the existing buffer */
@@ -48,7 +50,9 @@ APR_DECLARE(apr_status_t) apr_file_buffer_set(apr_
file->buffered = 0;
}
-apr_thread_mutex_unlock(file->mutex);
+if (file->flags & APR_FOPEN_XTHREAD) {
+apr_thread_mutex_unlock(file->mutex);
+}
return APR_SUCCESS;
}
Index: file_io/win32/open.c
===
--- file_io/win32/open.c(revision 1806645)
+++ file_io/win32/open.c(working copy)
@@ -452,8 +452,8 @@ APR_DECLARE(apr_status_t) apr_file_open(apr_file_t
(*new)->buffer = apr_palloc(pool, APR_FILE_DEFAULT_BUFSIZE);
(*new)->bufsize = APR_FILE_DEFAULT_BUFSIZE;
}
-/* Need the mutex to handled buffered and O_APPEND style file i/o */
-if ((*new)->buffered || (*new)->append) {
+/* Need the mutex to share an apr_file_t across multiple threads */
+if (flag & APR_FOPEN_XTHREAD) {
rv = apr_thread_mutex_create(&(*new)->mutex,
APR_THREAD_MUTEX_DEFAULT, pool);
if (rv) {
@@ -645,8 +645,7 @@ APR_DECLARE(apr_status_t) apr_os_file_put(apr_file
(*file)->buffer = apr_palloc(pool, APR_FILE_DEFAULT_BUFSIZE);
(*file)->bufsize = APR_FILE_DEFAULT_BUFSIZE;
}
-
-if ((*file)->append || (*file)->buffered) {
+if (flags & APR_FOPEN_XTHREAD) {
apr_status_t rv;
rv = apr_thread_mutex_create(&(*file)->mutex,
APR_THREAD_MUTEX_DEFAULT, pool);
Index: file_io/win32/readwrite.c
===
--- file_io/win32/readwrite.c (revision 1806645)
+++ file_io/win32/readwrite.c (working copy)
@@ -412,20 +412,26 @@ APR_DECLARE(apr_status_t) apr_file_write(apr_file_
if (thefile->append) {
apr_off_t offset = 0;
-/* apr_file_lock will mutex the file across processes.
- * The call to apr_thread_mutex_lock is added to avoid
- * a race condition between LockFile and WriteFile
- * that occasionally leads to deadlocked threads.
- */
-apr_thread_mutex_lock(thefile->mutex);
+if (thefile->flags & APR_FOPEN_XTHREAD) {
+/* apr_file_lock will mutex the file across processes.
+ * The call to apr_thread_mutex_lock is added to avoid
+ * a race condition between LockFile and WriteFile
+ * that occasionally leads to deadlocked threads.
+ */
+apr_thread_mutex_lock(thefile->mutex);
+
[PATCH] Fix a deadlock when appending to locked files on Windows (PR50058)
Hi everyone,
Currently, appending data to a file opened with APR_FOPEN_APPEND
that has been locked by apr_file_lock() will cause a deadlock on Windows.
[See PR50058, https://bz.apache.org/bugzilla/show_bug.cgi?id=50058]
This issue happens because atomic O_APPEND-style appends on Windows
are implemented using file locks. An append happens while holding the
file lock acquired with LockFile(), which is required to avoid a race
condition between seeking to the end of file and writing data. The
race is possible when multiple threads or processes are appending
data to the same file.
This approach causes a deadlock if the file has been previously locked
with apr_file_lock(). (Note that it's perfectly legit to lock the file or
its portion and perform the append after that.)
Apart from this, using file locks for file appends impacts their speed and
robustness. There is an overhead associated with locking the file, especially
if the file is not local. The robustness is affected, because other writes
to the same file may fail due to it being locked. Also, if a process is
terminated in the middle of the append operation, it might take some time
for the OS to release the file lock. During this time, the file would be
inaccessible to other processes. This may affect applications such as
httpd (with a multi-process MPM) that use APR_FOPEN_APPEND files for
logging.
This patch series fixes the issue by switching to the documented way to
atomically append data with a single WriteFile() call. It requires passing
special OVERLAPPED.Offset and OffsetHigh values (0x). On the
ZwWriteFile() layer, this maps to a FILE_WRITE_TO_END_OF_FILE constant
that instructs the OS (the corresponding file system driver) to write data
to the file's end.
Note that this approach is only used for files opened for synchronous I/O
because in this case the I/O Manager maintains the current file position.
Otherwise, the file offset returned or changed by the SetFilePointer() API is
not guaranteed to be valid and that could, for instance, break apr_file_seek()
calls after appending data. Sadly, if a file is opened for asynchronous I/O,
this call to WriteFile() doesn't update the OVERLAPPED.Offset member to
reflect the actual offset used when appending the data (which we could
then use to make seeking and other operations involving filePtr work).
Therefore, when appending to files opened for asynchronous I/O, we still
use the old LockFile + SetFilePointer + WriteFile approach.
Additional details on this can be found in:
https://msdn.microsoft.com/en-us/library/windows/desktop/aa365747
https://msdn.microsoft.com/en-us/library/windows/hardware/ff567121
The implementation is split into four dependent patches. The first three
patches are minor refactorings to the apr_file_write() function and lay the
necessary groundwork for the core change. The fourth patch is the core
change that fixes the issue in the described way and also includes the
tests.
The log messages are included in the beginning of each patch file.
Thanks,
Evgeny Kotkov
Minor refactoring of the Win32 file write code.
* file_io/win32/readwrite.c
(apr_file_write): Reuse the existing 'rv' variable to store the
status code. Reduce the scope of the 'offset' variable.
Patch by: Evgeny Kotkov
Index: file_io/win32/readwrite.c
===
--- file_io/win32/readwrite.c (revision 1806361)
+++ file_io/win32/readwrite.c (working copy)
@@ -372,9 +372,9 @@ APR_DECLARE(apr_status_t) apr_file_write(apr_file_
return rv;
} else {
if (!thefile->pipe) {
-apr_off_t offset = 0;
-apr_status_t rc;
if (thefile->append) {
+apr_off_t offset = 0;
+
/* apr_file_lock will mutex the file across processes.
* The call to apr_thread_mutex_lock is added to avoid
* a race condition between LockFile and WriteFile
@@ -381,15 +381,15 @@ APR_DECLARE(apr_status_t) apr_file_write(apr_file_
* that occasionally leads to deadlocked threads.
*/
apr_thread_mutex_lock(thefile->mutex);
-rc = apr_file_lock(thefile, APR_FLOCK_EXCLUSIVE);
-if (rc != APR_SUCCESS) {
+rv = apr_file_lock(thefile, APR_FLOCK_EXCLUSIVE);
+if (rv != APR_SUCCESS) {
apr_thread_mutex_unlock(thefile->mutex);
-return rc;
+return rv;
}
-rc = apr_file_seek(thefile, APR_END, );
-if (rc != APR_SUCCESS) {
+rv = apr_file_seek(thefile, APR_END, );
+if (rv != APR_SUCCESS) {
apr_thread_mutex_unlock(thefile->mutex);
-return rc;
+return rv;
}
}
if (thefile->pOverlapped) {
Re: [PATCH] Reduce the amount of WriteFile() syscalls when writing to buffered files
Evgeny Kotkov <evgeny.kot...@visualsvn.com> writes:
> In the meanwhile, apparently, there is an oversight in the core V1 patch
> (3-reduce-syscalls-for-buffered-writes-v1.patch.txt):
>
> If the buffer is not empty, and the caller issues a write with a chunk
> that slightly exceeds the buffer size, for example, 4100 bytes, it would
> result in flushing the buffer _and_ writing the remaining couple of bytes
> with WriteFile(). An appropriate thing to do here would be to flush the
> buffer, but put the few remaining bytes into the buffer, instead of writing
> them to disk and thus making an unnecessary syscall.
>
> With all this in mind, I will prepare the V2 version of the core patch.
I have attached the V2 version of the core patch.
To solve the aforementioned oversight in the V1 patch, I implemented the
optimization in a slightly different manner, by keeping the existing loop
but also handling a condition where we can write the remaining data with
a single WriteFile() call. Apart from this, the V2 patch also includes an
additional test, test_small_and_large_writes_buffered().
Speaking of the discussed idea of adjusting the strategy to avoid a memcpy()
with the write pattern like
WriteFile(13)
WriteFile(86267)
...
instead of
WriteFile(4096)
WriteFile(82185)
...
I preferred to keep the latter approach which keeps the minimum size of
the WriteFile() chunk equal to 4096, for the following reasons:
- My benchmarks do not show that the alternative pattern that also avoids
a memcpy() results in a quantifiable performance improvement.
- The existing code had a property that all WriteFile() calls, except
for maybe the last one, were performed in chunks with sizes that are
never less than 4096. Although I can't reproduce this in my environment,
it could be that introducing a possibility of writing in smaller chunks
would result in the decreased performance with specific hardware, non-
file handles or in situations when the OS write caching is disabled.
Therefore, currently, I think that it would be better to keep this
existing property.
- Probably, implementing the first approach would result in a bit more
complex code, as I think that it would require introducing an additional
if-else code path.
The log message is included in the beginning of the patch file.
Thanks,
Evgeny Kotkov
Win32: Improve apr_file_write() performance on buffered files by reducing
the amount of WriteFile() calls for large writes.
Previously, writing has been implemented with a loop that keeps copying the
data to the internal 4KB buffer and writing this buffer to disk by calling
WriteFile(4096). This patch reduces the amount of syscalls for large writes
by performing them with a single syscall, if possible. If the buffer is
not empty at the moment when the large write occurs, it is first filled
up to its 4KB capacity, flushed, and the remaining part of the data is
written with a single syscall.
* file_io/win32/readwrite.c
(write_buffered): Within the write loop, check if we have a situation
with an empty buffer and a large chunk pending to be written. In this
case, bypass the buffering and write the remaining chunk with a single
syscall. Return an appropriate number of written bytes to satisfy
the apr_file_write() function contract.
(apr_file_write): Adjust call to write_buffered().
* test/testfile.c
(test_large_write_buffered,
test_two_large_writes_buffered,
test_small_and_large_writes_buffered,
test_write_buffered_spanning_over_bufsize): New tests.
(testfile): Run the new tests.
Patch by: Evgeny Kotkov
Index: file_io/win32/readwrite.c
===
--- file_io/win32/readwrite.c (revision 1805607)
+++ file_io/win32/readwrite.c (working copy)
@@ -290,9 +290,8 @@ static apr_status_t write_helper(HANDLE filehand,
}
static apr_status_t write_buffered(apr_file_t *thefile, const char *buf,
- apr_size_t len)
+ apr_size_t len, apr_size_t *pwritten)
{
-apr_size_t blocksize;
apr_status_t rv;
if (thefile->direction == 0) {
@@ -306,20 +305,41 @@ static apr_status_t write_buffered(apr_file_t *the
thefile->direction = 1;
}
-rv = 0;
-while (rv == 0 && len > 0) {
-if (thefile->bufpos == thefile->bufsize) /* write buffer is full */
+*pwritten = 0;
+
+while (len > 0) {
+if (thefile->bufpos == thefile->bufsize) { /* write buffer is full */
rv = apr_file_flush(thefile);
+if (rv) {
+return rv;
+}
+}
+/* If our buffer is empty, and we cannot fit the remaining chunk
+ * into it, write the chunk with a single syscall and return.
+ */
+if (thefile->bufpos == 0 && len > thef
Re: [PATCH] Reduce the amount of WriteFile() syscalls when writing to buffered files
Markus Schaber <m.scha...@codesys.com> writes: > Hi, Evgeny, > > Great work, IMHO. Thank you :-) >> This patch series reduces the amount of syscalls in such situations by >> performing a single WriteFile() call without any buffering, if possible. >> If some data is already buffered, then the buffer is first filled, flushed >> and the remaining part of the data is written with a single WriteFile(). > > Why first refilling the buffer, instead of flushing the partial buffer > content, and then the original data to write? > > From my first glance, it looks like the number of syscalls should be > the same, and we could skip the mem copy? My original intention here was to avoid issuing small and non-4K-sized writes, which could happen if we don't refill the buffer. For instance, in the example above doing so would result in WriteFile(13) WriteFile(86267) WriteFile(13) WriteFile(86400) ... instead of WriteFile(4096) WriteFile(82185) WriteFile(4096) WriteFile(82317) ... But I should also say that I ran a couple of tests and I can't see any measurable performance difference between these two approaches, so it might indeed make sense to switch to the first of them and avoid a memcpy(). In the meanwhile, apparently, there is an oversight in the core V1 patch (3-reduce-syscalls-for-buffered-writes-v1.patch.txt): If the buffer is not empty, and the caller issues a write with a chunk that slightly exceeds the buffer size, for example, 4100 bytes, it would result in flushing the buffer _and_ writing the remaining couple of bytes with WriteFile(). An appropriate thing to do here would be to flush the buffer, but put the few remaining bytes into the buffer, instead of writing them to disk and thus making an unnecessary syscall. With all this in mind, I will prepare the V2 version of the core patch. Thanks, Evgeny Kotkov
[PATCH] Reduce the amount of WriteFile() syscalls when writing to buffered files
Hi everyone,
Currently, apr_file_write() can cause an excessive amount of syscalls
for buffered files on Windows in some situations. This happens because
for buffered files, writing is implemented with a loop that keeps copying
the data to the internal 4KB buffer and writing the contents of this
buffer to disk with WriteFile(). In other words, performing a 100 KB
write currently results in 25 consecutive WriteFile(4096) syscalls.
This patch series reduces the amount of syscalls in such situations by
performing a single WriteFile() call without any buffering, if possible.
If some data is already buffered, then the buffer is first filled, flushed
and the remaining part of the data is written with a single WriteFile().
My measurements indicate that writing in larger chunks and avoiding
such syscalls can save a certain part of the CPU time. For example,
"svn import" does both small and large buffered writes with the following
pattern:
write: nbytes = 9
write: nbytes = 5
write: nbytes = 86267
write: nbytes = 9
write: nbytes = 5
write: nbytes = 86413
write: nbytes = 9
write: nbytes = 5
write: nbytes = 86415
write: nbytes = 9
write: nbytes = 5
write: nbytes = 67680
...
When I test "svn import" for a large file before and after this patch, I
see that
- the amount of syscalls decreases from 199,403 to 17,061 and
- the overall CPU time decreases from 7.28 s to 6.68 s. (I think that's
a lot, considering that "svn import" does a many other CPU-intensive
operations.)
The implementation is split into three dependent patches. The first two
patches lay the necessary groundwork by factoring out a couple of helper
functions. The third patch is the core change that implements the
described optimization.
The log messages are included in the beginning of each patch file.
Thanks,
Evgeny Kotkov
Factor out a helper function that adapts WriteFile() to apr_size_t
instead of DWORD.
* file_io/win32/readwrite.c
(write_helper): New helper function, factored out from ...
(apr_file_flush): ...here.
Patch by: Evgeny Kotkov
Index: file_io/win32/readwrite.c
===
--- file_io/win32/readwrite.c (revision 1805607)
+++ file_io/win32/readwrite.c (working copy)
@@ -257,6 +257,38 @@ APR_DECLARE(apr_status_t) apr_file_rotating_manual
return APR_ENOTIMPL;
}
+/* Helper function that adapts WriteFile() to apr_size_t instead
+ * of DWORD. */
+static apr_status_t write_helper(HANDLE filehand, const char *buf,
+ apr_size_t len, apr_size_t *pwritten)
+{
+apr_size_t remaining = len;
+
+*pwritten = 0;
+do {
+DWORD to_write;
+DWORD written;
+
+if (remaining > APR_DWORD_MAX) {
+to_write = APR_DWORD_MAX;
+}
+else {
+to_write = (DWORD)remaining;
+}
+
+if (!WriteFile(filehand, buf, to_write, , NULL)) {
+*pwritten += written;
+return apr_get_os_error();
+}
+
+*pwritten += written;
+remaining -= written;
+buf += written;
+} while (remaining);
+
+return APR_SUCCESS;
+}
+
APR_DECLARE(apr_status_t) apr_file_write(apr_file_t *thefile, const void *buf,
apr_size_t *nbytes)
{
apr_status_t rv;
@@ -576,35 +608,15 @@ APR_DECLARE(apr_status_t) apr_file_gets(char *str,
APR_DECLARE(apr_status_t) apr_file_flush(apr_file_t *thefile)
{
if (thefile->buffered) {
-DWORD numbytes, written = 0;
apr_status_t rc = 0;
-char *buffer;
-apr_size_t bytesleft;
if (thefile->direction == 1 && thefile->bufpos) {
-buffer = thefile->buffer;
-bytesleft = thefile->bufpos;
+apr_size_t written;
-do {
-if (bytesleft > APR_DWORD_MAX) {
-numbytes = APR_DWORD_MAX;
-}
-else {
-numbytes = (DWORD)bytesleft;
-}
+rc = write_helper(thefile->filehand, thefile->buffer,
+ thefile->bufpos, );
+thefile->filePtr += written;
-if (!WriteFile(thefile->filehand, buffer, numbytes, ,
NULL)) {
-rc = apr_get_os_error();
-thefile->filePtr += written;
-break;
-}
-
-thefile->filePtr += written;
-bytesleft -= written;
-buffer += written;
-
-} while (bytesleft > 0);
-
if (rc == 0)
thefile->bufpos = 0;
}
Factor out a helper function that contains the implementation of writing
to buffered files.
* file_io/win32/readwrite.c
(write_buffered): New helper function, factored out from ...
Re: [PATCH] Allow larger apr_socket_listen() backlog queue lengths on Windows 8+
Evgeny Kotkov <evgeny.kot...@visualsvn.com> writes:
> The attached patch starts using SOMAXCONN_HINT() in the Win32
> implementation of the apr_socket_listen() function if this feature is
> supported by the underlying version of Windows.
With a bit more thought on this, here is the V2 version of this patch that
doesn't require having a new Windows SDK to get the new behavior.
This is achieved by optionally borrowing the definition of SOMAXCONN_HINT()
from an appropriate version of the SDK.
Regards,
Evgeny Kotkov
apr_socket_listen(): Allow larger backlog queue lengths on Windows 8+.
Starting with Windows 8, the socket listen() function accepts a special
SOMAXCONN_HINT(N) argument that allows making the backlog queue
length larger than the otherwise predefined limit of around 200:
https://msdn.microsoft.com/en-us/library/windows/desktop/ms739168
https://blogs.msdn.microsoft.com/winsdk/2015/06/01/winsocks-listen-backlog-offers-more-flexibility-in-windows-8/
Having a larger listen backlog can be used for certain high performance
applications that need to handle lots of incoming connections. One
example would be the httpd server with it's "ListenBacklog" directive
where setting it to a larger value currently allows serving more concurrent
connections on Windows with mpm_winnt.
* include/arch/win32/apr_arch_misc.h
(enum apr_oslevel_e): Add APR_WIN_8.
* misc/win32/misc.c
(apr_get_oslevel): Determine whether we are running on Windows 7 or
on Windows 8+.
* network_io/win32/sockets.c
(SOMAXCONN_HINT): Define this macro in case we are building against
an older version of Windows SDK.
(apr_socket_listen): Use SOMAXCONN_HINT() for the backlog queue length
if it's supported by the Windows version we are running on.
Patch by: Evgeny Kotkov
Index: include/arch/win32/apr_arch_misc.h
===
--- include/arch/win32/apr_arch_misc.h (revision 1801660)
+++ include/arch/win32/apr_arch_misc.h (working copy)
@@ -105,7 +105,8 @@ typedef enum {
APR_WIN_XP_SP2 = 62,
APR_WIN_2003 = 70,
APR_WIN_VISTA =80,
-APR_WIN_7 = 90
+APR_WIN_7 = 90,
+APR_WIN_8 = 100
} apr_oslevel_e;
extern APR_DECLARE_DATA apr_oslevel_e apr_os_level;
Index: misc/win32/misc.c
===
--- misc/win32/misc.c (revision 1801660)
+++ misc/win32/misc.c (working copy)
@@ -99,8 +99,10 @@ apr_status_t apr_get_oslevel(apr_oslevel_e *level)
else if (oslev.dwMajorVersion == 6) {
if (oslev.dwMinorVersion == 0)
apr_os_level = APR_WIN_VISTA;
+else if (oslev.dwMinorVersion == 1)
+apr_os_level = APR_WIN_7;
else
-apr_os_level = APR_WIN_7;
+apr_os_level = APR_WIN_8;
}
else {
apr_os_level = APR_WIN_XP;
Index: network_io/win32/sockets.c
===
--- network_io/win32/sockets.c (revision 1801660)
+++ network_io/win32/sockets.c (working copy)
@@ -24,6 +24,13 @@
#include "apr_arch_inherit.h"
#include "apr_arch_misc.h"
+/* Borrow the definition of SOMAXCONN_HINT() from Windows SDK 8,
+ * in case the SDK we are building against doesn't have it.
+ */
+#ifndef SOMAXCONN_HINT
+#define SOMAXCONN_HINT(b) (-(b))
+#endif
+
static char generic_inaddr_any[16] = {0}; /* big enough for IPv4 or IPv6 */
static apr_status_t socket_cleanup(void *sock)
@@ -223,7 +230,21 @@ APR_DECLARE(apr_status_t) apr_socket_bind(apr_sock
APR_DECLARE(apr_status_t) apr_socket_listen(apr_socket_t *sock,
apr_int32_t backlog)
{
-if (listen(sock->socketdes, backlog) == SOCKET_ERROR)
+int backlog_val;
+
+if (apr_os_level >= APR_WIN_8) {
+/* Starting from Windows 8, listen() accepts a special SOMAXCONN_HINT()
+ * arg that allows setting the listen backlog value to a larger
+ * value than the predefined Winsock 2 limit (several hundred).
+ *
https://blogs.msdn.microsoft.com/winsdk/2015/06/01/winsocks-listen-backlog-offers-more-flexibility-in-windows-8/
+ */
+backlog_val = SOMAXCONN_HINT(backlog);
+}
+else {
+backlog_val = backlog;
+}
+
+if (listen(sock->socketdes, backlog_val) == SOCKET_ERROR)
return apr_get_netos_error();
else
return APR_SUCCESS;
[PATCH] Allow larger apr_socket_listen() backlog queue lengths on Windows 8+
Hi everyone,
Starting with Windows 8, the socket listen() function accepts a special
SOMAXCONN_HINT(N) argument that allows making the backlog queue
length larger than the otherwise predefined limit of around 200:
https://msdn.microsoft.com/en-us/library/windows/desktop/ms739168
https://blogs.msdn.microsoft.com/winsdk/2015/06/01/winsocks-listen-backlog-offers-more-flexibility-in-windows-8/
Having a larger listen backlog can be used for certain high performance
applications that need to handle lots of incoming connections. One
example would be the httpd server with it's "ListenBacklog" directive
where setting it to a larger value currently allows serving more concurrent
connections on Windows with mpm_winnt.
The attached patch starts using SOMAXCONN_HINT() in the Win32
implementation of the apr_socket_listen() function if this feature is
supported by the underlying version of Windows.
The log message is included in the beginning of the patch file.
Regards,
Evgeny Kotkov
apr_socket_listen(): Allow larger backlog queue lengths on Windows 8+.
Starting with Windows 8, the socket listen() function accepts a special
SOMAXCONN_HINT(N) argument that allows making the backlog queue
length larger than the otherwise predefined limit of around 200:
https://msdn.microsoft.com/en-us/library/windows/desktop/ms739168
https://blogs.msdn.microsoft.com/winsdk/2015/06/01/winsocks-listen-backlog-offers-more-flexibility-in-windows-8/
Having a larger listen backlog can be used for certain high performance
applications that need to handle lots of incoming connections. One
example would be the httpd server with it's "ListenBacklog" directive
where setting it to a larger value currently allows serving more concurrent
connections on Windows with mpm_winnt.
* include/arch/win32/apr_arch_misc.h
(enum apr_oslevel_e): Add APR_WIN_8.
* misc/win32/misc.c
(apr_get_oslevel): Determine whether we are running on Windows 7 or
on Windows 8+.
* network_io/win32/sockets.c
(apr_socket_listen): Use SOMAXCONN_HINT() if it's supported by both
the SDK we are building against and the Windows version we are
running on.
Patch by: Evgeny Kotkov
Index: include/arch/win32/apr_arch_misc.h
===
--- include/arch/win32/apr_arch_misc.h (revision 1801587)
+++ include/arch/win32/apr_arch_misc.h (working copy)
@@ -105,7 +105,8 @@ typedef enum {
APR_WIN_XP_SP2 = 62,
APR_WIN_2003 = 70,
APR_WIN_VISTA =80,
-APR_WIN_7 = 90
+APR_WIN_7 = 90,
+APR_WIN_8 = 100
} apr_oslevel_e;
extern APR_DECLARE_DATA apr_oslevel_e apr_os_level;
Index: misc/win32/misc.c
===
--- misc/win32/misc.c (revision 1801587)
+++ misc/win32/misc.c (working copy)
@@ -99,8 +99,10 @@ apr_status_t apr_get_oslevel(apr_oslevel_e *level)
else if (oslev.dwMajorVersion == 6) {
if (oslev.dwMinorVersion == 0)
apr_os_level = APR_WIN_VISTA;
+else if (oslev.dwMinorVersion == 1)
+apr_os_level = APR_WIN_7;
else
-apr_os_level = APR_WIN_7;
+apr_os_level = APR_WIN_8;
}
else {
apr_os_level = APR_WIN_XP;
Index: network_io/win32/sockets.c
===
--- network_io/win32/sockets.c (revision 1801587)
+++ network_io/win32/sockets.c (working copy)
@@ -223,7 +223,20 @@ APR_DECLARE(apr_status_t) apr_socket_bind(apr_sock
APR_DECLARE(apr_status_t) apr_socket_listen(apr_socket_t *sock,
apr_int32_t backlog)
{
-if (listen(sock->socketdes, backlog) == SOCKET_ERROR)
+int backlog_val = backlog;
+
+#ifdef SOMAXCONN_HINT
+if (apr_os_level >= APR_WIN_8) {
+/* Starting from Windows 8, listen() accepts a special SOMAXCONN_HINT()
+ * arg that allows setting the listen backlog value to a larger
+ * value than the predefined Winsock 2 limit (several hundred).
+ *
https://blogs.msdn.microsoft.com/winsdk/2015/06/01/winsocks-listen-backlog-offers-more-flexibility-in-windows-8/
+ */
+backlog_val = SOMAXCONN_HINT(backlog);
+}
+#endif
+
+if (listen(sock->socketdes, backlog_val) == SOCKET_ERROR)
return apr_get_netos_error();
else
return APR_SUCCESS;
[PATCH] Fix apr_file_trunc() for buffered files on Windows and Unix
Hi everyone,
This patch fixes two issues with apr_file_trunc() for buffered files:
- The Win32 implementation incorrectly flushes the buffered writes _after_
truncating a file. Such files will have unexpected data written after
the position at which they should've been truncated. (Under Unix,
this issue has been fixed in https://svn.apache.org/r100)
- Both Win32 and Unix implementations incorrectly keep the data read into
a buffer after the file is truncated. Thus, reading from a file after
apr_file_trunc() can return invalid data from the previous file offset.
Log message:
[[[
Fix two issues with apr_file_trunc() for buffered files:
- The Win32 implementation incorrectly flushes the buffered writes _after_
truncating a file. Such files will have unexpected data written after
the position at which they should've been truncated. (Under Unix,
this issue has been fixed in https://svn.apache.org/r100)
- Both Win32 and Unix implementations incorrectly keep the data read into
a buffer after the file is truncated. Thus, reading from a file after
apr_file_trunc() can return invalid data from the previous file offset.
* file_io/win32/seek.c
(apr_file_trunc): Flush the write buffer or discard the read buffer
before truncating. Propely update the internal file offset (filePtr)
and the eof_hit marker.
* file_io/unix/seek.c
(apr_file_trunc): Discard the read buffer before truncating.
* test/testfile.c
(test_file_trunc): Extend the checks. Factor out part of this test...
(test_file_trunc_buffered_write): ...into this new test.
(test_file_trunc_buffered_write2, test_file_trunc_buffered_read): New tests.
(testfile): Run the new tests.
Patch by: Evgeny Kotkov
]]]
Regards,
Evgeny Kotkov
Index: file_io/unix/seek.c
===
--- file_io/unix/seek.c (revision 1784526)
+++ file_io/unix/seek.c (working copy)
@@ -117,6 +117,13 @@ apr_status_t apr_file_trunc(apr_file_t *fp, apr_of
/* Reset buffer positions for write mode */
fp->bufpos = fp->direction = fp->dataRead = 0;
}
+else if (fp->direction == 0) {
+/* Discard the read buffer, as we are about to reposition
+ * ourselves to the end of file.
+ */
+fp->bufpos = 0;
+fp->dataRead = 0;
+}
file_unlock(fp);
if (rc) {
return rc;
Index: file_io/win32/seek.c
===
--- file_io/win32/seek.c(revision 1784526)
+++ file_io/win32/seek.c(working copy)
@@ -161,17 +161,43 @@ APR_DECLARE(apr_status_t) apr_file_trunc(apr_file_
LONG offhi = (LONG)(offset >> 32);
DWORD rc;
+if (thefile->buffered) {
+if (thefile->direction == 1) {
+/* Figure out what needs to be flushed. Don't flush the part
+ * of the write buffer that will get truncated anyway.
+ */
+if (offset < thefile->filePtr) {
+thefile->bufpos = 0;
+}
+else if (offset < thefile->filePtr + (apr_off_t)thefile->bufpos) {
+thefile->bufpos = offset - thefile->filePtr;
+}
+
+if (thefile->bufpos != 0) {
+rv = apr_file_flush(thefile);
+if (rv != APR_SUCCESS)
+return rv;
+}
+}
+else if (thefile->direction == 0) {
+/* Discard the read buffer, as we are about to reposition
+ * ourselves to the end of file.
+ */
+thefile->bufpos = 0;
+thefile->dataRead = 0;
+}
+}
+
rc = SetFilePointer(thefile->filehand, offlo, , FILE_BEGIN);
if (rc == 0x)
if ((rv = apr_get_os_error()) != APR_SUCCESS)
return rv;
+thefile->filePtr = offset;
+/* Don't report EOF until the next read. */
+thefile->eof_hit = 0;
if (!SetEndOfFile(thefile->filehand))
return apr_get_os_error();
-if (thefile->buffered) {
-return setptr(thefile, offset);
-}
-
return APR_SUCCESS;
}
Index: test/testfile.c
===
--- test/testfile.c (revision 1784526)
+++ test/testfile.c (working copy)
@@ -822,10 +822,10 @@ static void test_file_trunc(abts_case *tc, void *d
const char *s;
apr_size_t nbytes;
apr_finfo_t finfo;
+char c;
apr_file_remove(fname, p);
-/* Test unbuffered */
rv = apr_file_open(, fname,
APR_FOPEN_CREATE | APR_FOPEN_READ |
APR_FOPEN_WRITE,
@@ -839,15 +839,40 @@ static void test_file_trunc(abts_case *tc, void *d
ABTS_SIZE_EQUAL(tc, strlen(s), nbytes);
rv = apr_file_trunc(f, 4);
ABTS_INT_EQUAL(tc, A
[PATCH] Optimize apr_file_gets() for buffered files on Windows
Hi everyone,
The Unix implementation of apr_file_gets() features an optimization
for buffered files that avoids calling apr_file_read() / memcpy() for
one byte, and directly uses the buffered data. This optimization was
added in https://svn.apache.org/r65294
The attached patch implements the same optimization in the Win32 version
of apr_file_gets(), and extends the corresponding part of the test suite.
This makes the function roughly 4 times faster than before. It is heavily
used by the Subversion's filesystem layer and various modules in the
HTTP Server, e.g., mod_negotiation, and these or other existing callers
would benefit from this optimization.
Log message:
[[[
Win32: Improve apr_file_gets() performance on buffered files by not calling
apr_file_read() on each byte.
The benchmark shows that this makes the function roughly 4 times faster:
4.202 ms -> 1.042 ms (I measured multiple calls for the same test file)
Also see https://svn.apache.org/r65294
* file_io/win32/readwrite.c
(apr_file_read): Factor out the part of this function that handles
reading from buffered files ...
(read_buffered): ...into this new helper.
(apr_file_gets): Use the buffer directly for buffered files, the same
way as in the Unix implementation.
* test/testfile.c
(test_gets, test_gets_buffered): Extend these tests with read-after-EOF
checks.
(test_gets_empty, test_gets_multiline, test_gets_small_buf,
test_gets_ungetc, test_gets_buffered_big): New tests.
(testfile): Run the new tests.
Patch by: Evgeny Kotkov
]]]
Regards,
Evgeny Kotkov
Index: file_io/win32/readwrite.c
===
--- file_io/win32/readwrite.c (revision 1781470)
+++ file_io/win32/readwrite.c (working copy)
@@ -140,6 +140,56 @@ static apr_status_t read_with_timeout(apr_file_t *
return rv;
}
+static apr_status_t read_buffered(apr_file_t *thefile, void *buf, apr_size_t
*len)
+{
+apr_status_t rv;
+char *pos = (char *)buf;
+apr_size_t blocksize;
+apr_size_t size = *len;
+
+if (thefile->direction == 1) {
+rv = apr_file_flush(thefile);
+if (rv != APR_SUCCESS) {
+return rv;
+}
+thefile->bufpos = 0;
+thefile->direction = 0;
+thefile->dataRead = 0;
+}
+
+rv = 0;
+while (rv == 0 && size > 0) {
+if (thefile->bufpos >= thefile->dataRead) {
+apr_size_t read;
+rv = read_with_timeout(thefile, thefile->buffer,
+ thefile->bufsize, );
+if (read == 0) {
+if (rv == APR_EOF)
+thefile->eof_hit = TRUE;
+break;
+}
+else {
+thefile->dataRead = read;
+thefile->filePtr += thefile->dataRead;
+thefile->bufpos = 0;
+}
+}
+
+blocksize = size > thefile->dataRead - thefile->bufpos ?
thefile->dataRead - thefile->bufpos : size;
+memcpy(pos, thefile->buffer + thefile->bufpos, blocksize);
+thefile->bufpos += blocksize;
+pos += blocksize;
+size -= blocksize;
+}
+
+*len = pos - (char *)buf;
+if (*len) {
+rv = APR_SUCCESS;
+}
+
+return rv;
+}
+
APR_DECLARE(apr_status_t) apr_file_read(apr_file_t *thefile, void *buf,
apr_size_t *len)
{
apr_status_t rv;
@@ -177,57 +227,10 @@ APR_DECLARE(apr_status_t) apr_file_read(apr_file_t
}
}
if (thefile->buffered) {
-char *pos = (char *)buf;
-apr_size_t blocksize;
-apr_size_t size = *len;
-
if (thefile->flags & APR_FOPEN_XTHREAD) {
apr_thread_mutex_lock(thefile->mutex);
}
-
-if (thefile->direction == 1) {
-rv = apr_file_flush(thefile);
-if (rv != APR_SUCCESS) {
-if (thefile->flags & APR_FOPEN_XTHREAD) {
-apr_thread_mutex_unlock(thefile->mutex);
-}
-return rv;
-}
-thefile->bufpos = 0;
-thefile->direction = 0;
-thefile->dataRead = 0;
-}
-
-rv = 0;
-while (rv == 0 && size > 0) {
-if (thefile->bufpos >= thefile->dataRead) {
-apr_size_t read;
-rv = read_with_timeout(thefile, thefile->buffer,
- thefile->bufsize, );
-if (read == 0) {
-if (rv == APR_EOF)
-thefile->eof_hit = TRUE;
-break;
-}
-else {
-thefile->dataRead = read;
-thefile->filePtr += thefile->dataRead;
-thefile->bufpos = 0;
-}
-}
-
-bloc
