Re: apreqXXXXXX temp files remain after processing uploads greater than 256kb. Further large upload fails
Steve Hay [EMAIL PROTECTED] writes:
If I just apply the util.c change (i.e. drop the NOCLEANUP and
SHARELOCK flags, but continue using perl.exe and httpd.exe for the
upload tests) then all the tests still seem to pass anyway, but I do
then see some stray apreqXX files left over in my temp directory
from time to time
I still believe the problem stems from mixing posix and win32 calls;
but it's perl that mixes them, not apr. In any case, we have evidence
that our cleanup is failing, so we should include code that traps the
error and tries to recover somehow.
How about leaving the current upload.t tests alone, so they can
generate tempfiles on Steve's box, and coming up with some cleanup
code that, at least while we're trying to sort this out, does
something like this:
static apr_status_t apreq_file_cleanup(void *d)
{
struct cleanup_data *data = d;
apr_status_t s = apr_file_remove(data-fname, data-pool);
#ifdef WIN32
if (s != APR_SUCCESS) {
apr_file_t *stderr;
apr_pool_t *p;
const char fmt[] = apr_file_remove failed with status %d on temp file
%s.
Sleeping for 1 sec before retrying;
apr_pool_create_ex(p, NULL, NULL, NULL);
apr_file_open_stderr(stderr, p);
apr_file_printf(stderr, fmt, s, data-fname);
sleep(1); /* may need to #include unistd.h */
apr_pool_clear(p);
apr_pool_destroy(p);
s = apr_file_remove(data-fname, data-pool);
}
#endif
return s;
}
--
Joe Schaefer
Re: apreqXXXXXX temp files remain after processing uploads greater than 256kb. Further large upload fails
Randy Kobes wrote: On Wed, 14 Mar 2007, Steve Hay wrote: I tried your patch with the current svn version (revision 518242), but I'm still seeing intermittent failures (usually in tests 15, 16 and/or 20) either when I run nmake test from the top-level, or when I run: perl -Iblib/arch -Iblib/lib t/TEST -verbose=1 t/apreq/upload.t from the glue/perl sub-directory :-( I'm running perl-5.8.8, apache-2.2.2 and mod_perl-2.0.3 (RC3, I think). Do I need to update anything there? I don't think so - I ran the tests against essentially the same setup, and didn't see any failures when run a number of times. Perhaps just to narrow things down, could you try the attached C file (a VC++ Makefile is also attached)? This uses the relevant parts of libapreq2 to create and cleanup a temp file; the Makefile produces 4 .exes: apr_temp: don't enable APR_FILE_NOCLEANUP nor APR_SHARELOCK apr_temp_nc: enable only APR_FILE_NOCLEANUP apr_temp_sh: enable only APR_SHARELOCK apr_temp_nc_sh: enable both APR_FILE_NOCLEANUP and APR_SHARELOCK These are run, for example apr_temp 20 which will go in a loop and create, and then remove, 20 temp files (with no arguments, the default is 10. Are there any problems with the cleanup in any of these? If not, then it looks like the problem is somewhere within the Perl glue. Apologies for the slow response. I ran each program with an argument of 500 for good measure. All four programs successfully create and then delete a large number (presumably 500, I didn't count 'em!) of apreq* temp files. apr_temp and apr_temp_sh both deleted all those files about as quickly as they were made (i.e. very quickly), while apr_temp_nc and apr_temp_nc_sh both output loads of CLEANUP messages in the console quickly but then seemed to hang for nearly a minute, during which time loads of apreq* files were visible in the temp directory, and then eventually exited and all the files disappeared. --
Re: apreqXXXXXX temp files remain after processing uploads greater than 256kb. Further large upload fails
Randy Kobes wrote: On Fri, 9 Mar 2007, Joe Schaefer wrote: Randy, do you know why we use the APR_FILE_NOCLEANUP flag? Maybe we should just remove that and see if it fixes the problem Vinay is seeing. Hi Steve, and all, If you remember from http://marc.theaimsgroup.com/?t=11533762941r=1w=2 there was a problem with stray temp files in apreq remaining after uploads; we came up with a fix that worked most of the time, but Vinay has come up with something that appears better, and more understandable. The attached diff against library/util.c (in the svn sources) works for me in getting multiple invocations of the glue/perl/t/apreq/upload.t to pass; if you have time, could you try it out to see how it fares on your system? Thanks. I tried your patch with the current svn version (revision 518242), but I'm still seeing intermittent failures (usually in tests 15, 16 and/or 20) either when I run nmake test from the top-level, or when I run: perl -Iblib/arch -Iblib/lib t/TEST -verbose=1 t/apreq/upload.t from the glue/perl sub-directory :-( I'm running perl-5.8.8, apache-2.2.2 and mod_perl-2.0.3 (RC3, I think). Do I need to update anything there? -- Radan Computational Ltd. The information contained in this message and any files transmitted with it are confidential and intended for the addressee(s) only. If you have received this message in error or there are any problems, please notify the sender immediately. The unauthorized use, disclosure, copying or alteration of this message is strictly forbidden. Note that any views or opinions presented in this email are solely those of the author and do not necessarily represent those of Radan Computational Ltd. The recipient(s) of this message should check it and any attached files for viruses: Radan Computational will accept no liability for any damage caused by any virus transmitted by this email.
Re: apreqXXXXXX temp files remain after processing uploads greater than 256kb. Further large upload fails
Steve Hay [EMAIL PROTECTED] writes: I tried your patch with the current svn version (revision 518242), but I'm still seeing intermittent failures (usually in tests 15, 16 and/or 20) either when I run nmake test from the top-level, or when I run: perl -Iblib/arch -Iblib/lib t/TEST -verbose=1 t/apreq/upload.t from the glue/perl sub-directory :-( Posting some diagnostic output might help. Anything in the error logs? Are you seeing any lingering tempfiles, and if so, are they prefixed with apreq (ie coming from apreq_file_mktemp), or are they from the test scripts? Randy, I see a lot of stuff like unlink $file if -f $file; in the upload.t tests, which might need to be changed to unlink $file or die Can't unlink $file: $!; -- Joe Schaefer
Re: apreqXXXXXX temp files remain after processing uploads greater than 256kb. Further large upload fails
On 3/12/07, Joe Schaefer [EMAIL PROTECTED] wrote:
Ok, lets presume DeleteFile() is failing, and passing
that error along to apr_file_remove(). In apreq_file_cleaup
let's then try what cygwin does in its unlink implementation:
opening the file again (using the apr API) with the APR_DELONCLOSE
flag set, then immediately closing it.
Here's a different suggestion to try:
Index: library/util.c
===
--- library/util.c (revision 517052)
+++ library/util.c (working copy)
@@ -816,10 +816,6 @@
return rc;
data = apr_palloc(pool, sizeof *data);
-/* cleanups are LIFO, so this one will run just after
- the cleanup set by mktemp */
-apr_pool_cleanup_register(pool, data,
- apreq_file_cleanup, apreq_file_cleanup);
/* NO APR_DELONCLOSE! see comment above */
flag = APR_CREATE | APR_READ | APR_WRITE | APR_EXCL | APR_BINARY;
@@ -828,10 +824,11 @@
* a grep through the httpd sources seems to indicate
* it's only used in sdbm files??
*/
-#ifdef WIN32
-flag |= APR_FILE_NOCLEANUP | APR_SHARELOCK;
-#endif
rc = apr_file_mktemp(fp, tmpl, flag, pool);
+/* cleanups are LIFO, so this one will run just before
+ the cleanup set by mktemp */
+apr_pool_cleanup_register(pool, data,
+ apreq_file_cleanup, apreq_file_cleanup);
if (rc == APR_SUCCESS) {
apr_file_name_get(data-fname, *fp);
In other words, delete the file *before* closing it.
This shouldn't matter. My version is apreq2-2.0.8 with Apache
2.2.2/2.2.4 build with Visual Studio 2003 running on Windows XP SP2
with all updates.
Here's what I get to see with filemon(from sysinternals) when I run
with my second patch(removing only the #ifdef win32 part):
** first apr_file_mktemp-apr_file_open-CreateFile **
12:33:59.625 AM httpd.exe:4032 IRP_MJ_CREATE
C:\DOCUME~1\pi\LOCALS~1\Temp\apreqq4EH3dSUCCESS Options: Create
Access: 0012019F
** then after all the reads and writes etc.. finally, apr's
file_cleanup-CloseHandle **
** presense of IRP_MJ_CLEANUP in here indicates that the filehandle
count is zero. **
12:35:29.281 AM httpd.exe:4032 IRP_MJ_CLEANUP
C:\DOCUME~1\pi\LOCALS~1\Temp\apreqq4EH3dSUCCESS
12:35:29.281 AM httpd.exe:4032 IRP_MJ_CREATE
C:\DOCUME~1\pi\LOCALS~1\Temp\apreqq4EH3dSUCCESS Options: Open
Access: 00120189
12:35:29.281 AM httpd.exe:4032 IRP_MJ_SET_INFORMATION
C:\DOCUME~1\pi\LOCALS~1\Temp\apreqq4EH3dSUCCESS
FileBasicInformation
12:35:29.281 AM System:4032 IRP_MJ_CLEANUP
C:\DOCUME~1\pi\LOCALS~1\Temp\apreqq4EH3dSUCCESS
12:35:29.281 AM System:4032 IRP_MJ_CLOSE
C:\DOCUME~1\pi\LOCALS~1\Temp\apreqq4EH3dSUCCESS
12:35:29.281 AM httpd.exe:4032 IRP_MJ_CREATE
C:\DOCUME~1\pi\LOCALS~1\Temp\apreqq4EH3dSUCCESS Options: Open
Access: 0180
12:35:29.281 AM System:4032 IRP_MJ_CLEANUP
C:\DOCUME~1\pi\LOCALS~1\Temp\apreqq4EH3dSUCCESS
12:35:29.281 AM System:4032 IRP_MJ_CLOSE
C:\DOCUME~1\pi\LOCALS~1\Temp\apreqq4EH3dSUCCESS
12:35:29.281 AM httpd.exe:4032 IRP_MJ_CLOSE
C:\DOCUME~1\pi\LOCALS~1\Temp\apreqq4EH3dSUCCESS
** After that apreq_file_cleanup-apr_file_remove-DeleteFile **
** DeleteFile does this: open the file of the given name, mark it for
deletion, close the file. **
12:38:10.218 AM httpd.exe:4032 IRP_MJ_CREATE
C:\DOCUME~1\pi\LOCALS~1\Temp\apreqq4EH3dSUCCESS Options: Open
Access: 00010080
12:38:10.218 AM httpd.exe:4032 IRP_MJ_SET_INFORMATION
C:\DOCUME~1\pi\LOCALS~1\Temp\apreqq4EH3dSUCCESS Delete
12:38:10.218 AM httpd.exe:4032 IRP_MJ_CLEANUP
C:\DOCUME~1\pi\LOCALS~1\Temp\apreqq4EH3dSUCCESS
12:38:10.218 AM httpd.exe:4032 IRP_MJ_CLOSE
C:\DOCUME~1\pi\LOCALS~1\Temp\apreqq4EH3dSUCCESS
Notice that interchanging the order of above two cleanup functinons
has no change in the final result. You either close the
file(CloseHandle) and then mark the file for deletion(DeleteFile) or
mark the file for deletion and then close the file, it's all the same.
The file will go if there were no other file handles(say, from inside
perl content handler that ran just before) holding on to it.
I don't know the perl code, but I would assume it would be doing all
it's stuff(open/copy/closing the temp file) before either of the two
cleanup functions are called above.
So, if we apply the patch suggested by Joe or me(second patch) and fix
the perl code which originally triggered all this discussion, all
should be fine.
Thanks,
Vinay Y S
Re: apreqXXXXXX temp files remain after processing uploads greater than 256kb. Further large upload fails
Joe Schaefer [EMAIL PROTECTED] writes: Randy Kobes [EMAIL PROTECTED] writes: [...] Removing the APR_FILE_NOCLEANUP would, I think, bring us back to the problem described at http://marc.theaimsgroup.com/?t=11533762941r=1w=2 for which this was introduced, in that some Win32 systems have occasional stray temp files lingering around. May I suggest that people start posting version numbers of both libapr and operating system? All we're doing now is running around blindly tweaking crap that we really shouldn't be tweaking in the first place. Apologies for the tone of that, I don't mean to sound so discouraging. Let me try to offer a suggestion for how we might deal with this constructively. The problem, as far as I can tell, stems from the fact that apr uses the win32 api for some things, and the posix api for others. On Windows those are two entirely different subsystems, with varying levels of quality depending on which version of Windows you are running. So I think what's happening in the cases where the tempfiles aren't being removed is that the call to apr_file_remove is failing. On windows, let's trap that error in apreq_file_cleanup and call DeleteFile() in that case. If that fails return APR_EGENERAL. Also, get rid of this ifdef in apreq_file_mktemp: #ifdef WIN32 flag |= APR_FILE_NOCLEANUP | APR_SHARELOCK; #endif It's bogus, and IMO is only confusing the situation. -- Joe Schaefer
Re: apreqXXXXXX temp files remain after processing uploads greater than 256kb. Further large upload fails
On Sun, 11 Mar 2007, Vinay Y S wrote: Actually, either my earlier patch which adds a apr_file_close in apreq_file_cleanup or just removing the APR_FILE_NOCLEANUP from the flags(patch attached) is enough. Both together isn't required. (but it's safe as the cleanup handler installed by apr would get uninstalled when you do apr_file_close). I favor doing just one of them, preferably, removing the APR_FILE_NOCLEANUP flag. This makes the WIN32 code identical to other platforms(where it's already working fine, so no change for them). Patch for the same against the svn source is given below. Please try this on win32 systems. Removing the APR_FILE_NOCLEANUP would, I think, bring us back to the problem described at http://marc.theaimsgroup.com/?t=11533762941r=1w=2 for which this was introduced, in that some Win32 systems have occasional stray temp files lingering around. -- best regards, Randy
Re: apreqXXXXXX temp files remain after processing uploads greater than 256kb. Further large upload fails
Randy Kobes [EMAIL PROTECTED] writes: On Sun, 11 Mar 2007, Vinay Y S wrote: Actually, either my earlier patch which adds a apr_file_close in apreq_file_cleanup or just removing the APR_FILE_NOCLEANUP from the flags(patch attached) is enough. Both together isn't required. (but it's safe as the cleanup handler installed by apr would get uninstalled when you do apr_file_close). I favor doing just one of them, preferably, removing the APR_FILE_NOCLEANUP flag. This makes the WIN32 code identical to other platforms(where it's already working fine, so no change for them). Patch for the same against the svn source is given below. Please try this on win32 systems. Removing the APR_FILE_NOCLEANUP would, I think, bring us back to the problem described at http://marc.theaimsgroup.com/?t=11533762941r=1w=2 for which this was introduced, in that some Win32 systems have occasional stray temp files lingering around. May I suggest that people start posting version numbers of both libapr and operating system? All we're doing now is running around blindly tweaking crap that we really shouldn't be tweaking in the first place. -- Joe Schaefer
Re: apreqXXXXXX temp files remain after processing uploads greater than 256kb. Further large upload fails
On 3/11/07, Randy Kobes [EMAIL PROTECTED] wrote:
On Fri, 9 Mar 2007, Joe Schaefer wrote:
Randy, do you know why we use the APR_FILE_NOCLEANUP flag? Maybe
we should just remove that and see if it fixes the problem Vinay
is seeing.
Hi Steve, and all,
If you remember from
http://marc.theaimsgroup.com/?t=11533762941r=1w=2
there was a problem with stray temp files in apreq
remaining after uploads; we came up with a fix that
worked most of the time, but Vinay has come up with
something that appears better, and more understandable.
The attached diff against library/util.c (in the svn
sources) works for me in getting multiple invocations
of the glue/perl/t/apreq/upload.t to pass; if you have
time, could you try it out to see how it fares on
your system? Thanks.
Actually, either my earlier patch which adds a apr_file_close in
apreq_file_cleanup or just removing the APR_FILE_NOCLEANUP from the
flags(patch attached) is enough. Both together isn't required. (but
it's safe as the cleanup handler installed by apr would get
uninstalled when you do apr_file_close). I favor doing just one of
them, preferably, removing the APR_FILE_NOCLEANUP flag. This makes the
WIN32 code identical to other platforms(where it's already working
fine, so no change for them). Patch for the same against the svn
source is given below. Please try this on win32 systems.
Thanks,
Vinay Y S
Index: library/util.c
===
--- library/util.c (revision 516861)
+++ library/util.c (working copy)
@@ -823,14 +823,6 @@
/* NO APR_DELONCLOSE! see comment above */
flag = APR_CREATE | APR_READ | APR_WRITE | APR_EXCL | APR_BINARY;
-/* Win32 needs the following to remove temp files.
- * XXX: figure out why the APR_SHARELOCK flag works;
- * a grep through the httpd sources seems to indicate
- * it's only used in sdbm files??
-*/
-#ifdef WIN32
-flag |= APR_FILE_NOCLEANUP | APR_SHARELOCK;
-#endif
rc = apr_file_mktemp(fp, tmpl, flag, pool);
if (rc == APR_SUCCESS) {
Index: library/util.c
===
--- library/util.c (revision 516861)
+++ library/util.c (working copy)
@@ -823,14 +823,6 @@
/* NO APR_DELONCLOSE! see comment above */
flag = APR_CREATE | APR_READ | APR_WRITE | APR_EXCL | APR_BINARY;
-/* Win32 needs the following to remove temp files.
- * XXX: figure out why the APR_SHARELOCK flag works;
- * a grep through the httpd sources seems to indicate
- * it's only used in sdbm files??
-*/
-#ifdef WIN32
-flag |= APR_FILE_NOCLEANUP | APR_SHARELOCK;
-#endif
rc = apr_file_mktemp(fp, tmpl, flag, pool);
if (rc == APR_SUCCESS) {
Re: apreqXXXXXX temp files remain after processing uploads greater than 256kb. Further large upload fails
Joe Schaefer [EMAIL PROTECTED] writes: Vinay Y S [EMAIL PROTECTED] writes: There is a problem with apreq temp file cleanup on win32. For each POST request with POST body greater than 256KB, TWO temp files of exact same size get created in temp directory and they are not deleted even after the request is handled. They get deleted when apache server is stopped. Thanks for the detailed report and patch! What version of apr are you using? The reason I ask is because whenever you open a file, normally a pool cleanup hook is registered to close it. Ah, I see now. I should have looked at the source before opening my mouth ;-): flag = APR_CREATE | APR_READ | APR_WRITE | APR_EXCL | APR_BINARY; /* Win32 needs the following to remove temp files. * XXX: figure out why the APR_SHARELOCK flag works; * a grep through the httpd sources seems to indicate * it's only used in sdbm files?? */ #ifdef WIN32 flag |= APR_FILE_NOCLEANUP | APR_SHARELOCK; #endif rc = apr_file_mktemp(fp, tmpl, flag, pool); Randy, do you know why we use the APR_FILE_NOCLEANUP flag? Maybe we should just remove that and see if it fixes the problem Vinay is seeing. -- Joe Schaefer
Re: apreqXXXXXX temp files remain after processing uploads greater than 256kb. Further large upload fails
Vinay Y S [EMAIL PROTECTED] writes: There is a problem with apreq temp file cleanup on win32. For each POST request with POST body greater than 256KB, TWO temp files of exact same size get created in temp directory and they are not deleted even after the request is handled. They get deleted when apache server is stopped. Thanks for the detailed report and patch! What version of apr are you using? The reason I ask is because whenever you open a file, normally a pool cleanup hook is registered to close it. apreq_file_mktemp() expects the pool cleanup implicit in the apr_file_mktemp() call to close the file, so I'm wondering why that's not happening in your situation. -- Joe Schaefer
Re: apreqXXXXXX temp files remain after processing uploads greater than 256kb. Further large upload fails
On 3/9/07, Joe Schaefer [EMAIL PROTECTED] wrote: flag = APR_CREATE | APR_READ | APR_WRITE | APR_EXCL | APR_BINARY; /* Win32 needs the following to remove temp files. * XXX: figure out why the APR_SHARELOCK flag works; * a grep through the httpd sources seems to indicate * it's only used in sdbm files?? */ #ifdef WIN32 flag |= APR_FILE_NOCLEANUP | APR_SHARELOCK; #endif rc = apr_file_mktemp(fp, tmpl, flag, pool); Randy, do you know why we use the APR_FILE_NOCLEANUP flag? Maybe we should just remove that and see if it fixes the problem Vinay is seeing. Yes, removing APR_FILE_NOCLEANUP does exactly the same thing as my patch. Rather, we can remove the whole #ifdef WIN32 as APR_SHARELOCK isn't used for anything in file_io on win32 today. -- Vinay Y S
Re: apreqXXXXXX temp files remain after processing uploads greater than 256kb. Further large upload fails
On Sat, 10 Mar 2007, Vinay Y S wrote: On 3/9/07, Joe Schaefer [EMAIL PROTECTED] wrote: flag = APR_CREATE | APR_READ | APR_WRITE | APR_EXCL | APR_BINARY; /* Win32 needs the following to remove temp files. * XXX: figure out why the APR_SHARELOCK flag works; * a grep through the httpd sources seems to indicate * it's only used in sdbm files?? */ #ifdef WIN32 flag |= APR_FILE_NOCLEANUP | APR_SHARELOCK; #endif rc = apr_file_mktemp(fp, tmpl, flag, pool); Randy, do you know why we use the APR_FILE_NOCLEANUP flag? Maybe we should just remove that and see if it fixes the problem Vinay is seeing. Yes, removing APR_FILE_NOCLEANUP does exactly the same thing as my patch. Rather, we can remove the whole #ifdef WIN32 as APR_SHARELOCK isn't used for anything in file_io on win32 today. I'll have to look at this - there's a discussion at http://marc.theaimsgroup.com/?t=11533762941r=1w=2 of why this was introduced. I remember that this was a frustrating issue, as something that worked on one system in removing temp files didn't work on another (at least between Steve and myself). What was committed seemed to work most of the time for most systems, at least at that time. -- best regards, Randy
