Re: svn commit: r1856274 - in /apr/apr/branches/1.7.x: ./ configure.in file_io/unix/dir.c
I'm happy to do that, and to preserve your dir.c fix. Commit coming up, crossing fingers for a clean Linux maintainer compilation. On Sun, Mar 31, 2019, 11:44 Yann Ylavic wrote: > On Sun, Mar 31, 2019 at 5:29 PM William A Rowe Jr > wrote: > > > > On Sun, Mar 31, 2019, 09:15 Yann Ylavic wrote: > >> > >> On Sun, Mar 31, 2019 at 3:58 AM William A Rowe Jr > wrote: > >> > > >> > Let me just throw this out there and get your reaction, Yann, now that > >> > I'm back in town... The patch I was initially using to accomplish the > >> > equivalent was as simple as; > >> [] > >> > Seems much less wordy for a feature we agree should just be dropped > >> > altogether fairly soon. Thoughts? > >> > >> I don't know actually, I think it's not really "fair" to say that > >> readdir() is thread-safe, you can't really call it from different > >> threads with the same DIR arg, while you might with readdir_r() > >> (besides all its caveats). I'm talking about the system functions > >> here, not apr_dir_read() which is not thread-safe (on the same > >> apr_dir_t) with either underlying function, as we discussed elsewhere. > >> But who would do that anyway?! I think we should do[x]cument that just > in case. > >> > >> As for your patch vs mine, it depends on whether it exists systems > >> (that we still support) on which readdir() is really not thread-safe > >> even with different passed-in DIRs (e.g. the returned struct dirent is > >> static somewhere in the implementation, as opposed to some room > >> reserved in the struct DIR itself like in "modern" implementations), > >> and these systems provide readdir_r() for the "thread-safe" way. > >> I suppose such system wouldn't emit a warning for readdir_r(), my > >> patch addresses them while yours does not. > >> So the question is, does such system exists and do we want to still > support it? > > > > > > I think our answer is we never did support this. > > > > My thought is that all support this, inasmuch as they support concurrent > dlopen's and readdir's on the same thread. If they are truly not thread > safe (as opposed to parallel or reentrant) on different handles that would > be a very thread-unsafe architecture in the first place, no? A perfect > example for APR has no threads at all. > > > > Since our readdir is threadsafe flag has a well established meaning > (thread safe for APR's purpose alone) and we can preserve that meaning with > no harm between 1.0 and 1.x builds (all overrides preserve the desired > behavior) I'm in favor of the simplest solution. > > > > I'm entirely in favor of dropping the macro from 2.0 and never returning > to dirread_r() after all the explanatory docs and your evaluation of APR. > If user wants to walk a single directory across parallel threads, they > clearly need to serialize the actual directory read and use a serialized > allocator in that odd edge case, much as they must do the same for any sane > file read. > > Make sense, I'm fine with this. > If you want to proceed by reverting my related changes, please go down > to r1789258 since this first commit was an (unsatisfactory) try later > overwritten by r1856189. The comment change from r1789258 might be > preserved though. > Thanks for the simplification! >
Re: svn commit: r1856274 - in /apr/apr/branches/1.7.x: ./ configure.in file_io/unix/dir.c
On Sun, Mar 31, 2019 at 5:29 PM William A Rowe Jr wrote: > > On Sun, Mar 31, 2019, 09:15 Yann Ylavic wrote: >> >> On Sun, Mar 31, 2019 at 3:58 AM William A Rowe Jr >> wrote: >> > >> > Let me just throw this out there and get your reaction, Yann, now that >> > I'm back in town... The patch I was initially using to accomplish the >> > equivalent was as simple as; >> [] >> > Seems much less wordy for a feature we agree should just be dropped >> > altogether fairly soon. Thoughts? >> >> I don't know actually, I think it's not really "fair" to say that >> readdir() is thread-safe, you can't really call it from different >> threads with the same DIR arg, while you might with readdir_r() >> (besides all its caveats). I'm talking about the system functions >> here, not apr_dir_read() which is not thread-safe (on the same >> apr_dir_t) with either underlying function, as we discussed elsewhere. >> But who would do that anyway?! I think we should do[x]cument that just in >> case. >> >> As for your patch vs mine, it depends on whether it exists systems >> (that we still support) on which readdir() is really not thread-safe >> even with different passed-in DIRs (e.g. the returned struct dirent is >> static somewhere in the implementation, as opposed to some room >> reserved in the struct DIR itself like in "modern" implementations), >> and these systems provide readdir_r() for the "thread-safe" way. >> I suppose such system wouldn't emit a warning for readdir_r(), my >> patch addresses them while yours does not. >> So the question is, does such system exists and do we want to still support >> it? > > > I think our answer is we never did support this. > > My thought is that all support this, inasmuch as they support concurrent > dlopen's and readdir's on the same thread. If they are truly not thread safe > (as opposed to parallel or reentrant) on different handles that would be a > very thread-unsafe architecture in the first place, no? A perfect example for > APR has no threads at all. > > Since our readdir is threadsafe flag has a well established meaning (thread > safe for APR's purpose alone) and we can preserve that meaning with no harm > between 1.0 and 1.x builds (all overrides preserve the desired behavior) I'm > in favor of the simplest solution. > > I'm entirely in favor of dropping the macro from 2.0 and never returning to > dirread_r() after all the explanatory docs and your evaluation of APR. If > user wants to walk a single directory across parallel threads, they clearly > need to serialize the actual directory read and use a serialized allocator in > that odd edge case, much as they must do the same for any sane file read. Make sense, I'm fine with this. If you want to proceed by reverting my related changes, please go down to r1789258 since this first commit was an (unsatisfactory) try later overwritten by r1856189. The comment change from r1789258 might be preserved though. Thanks for the simplification!
Re: svn commit: r1856274 - in /apr/apr/branches/1.7.x: ./ configure.in file_io/unix/dir.c
On Sun, Mar 31, 2019 at 3:58 AM William A Rowe Jr wrote: > > Let me just throw this out there and get your reaction, Yann, now that > I'm back in town... The patch I was initially using to accomplish the > equivalent was as simple as; [] > Seems much less wordy for a feature we agree should just be dropped > altogether fairly soon. Thoughts? I don't know actually, I think it's not really "fair" to say that readdir() is thread-safe, you can't really call it from different threads with the same DIR arg, while you might with readdir_r() (besides all its caveats). I'm talking about the system functions here, not apr_dir_read() which is not thread-safe (on the same apr_dir_t) with either underlying function, as we discussed elsewhere. But who would do that anyway?! I think we should do[x]cument that just in case. As for your patch vs mine, it depends on whether it exists systems (that we still support) on which readdir() is really not thread-safe even with different passed-in DIRs (e.g. the returned struct dirent is static somewhere in the implementation, as opposed to some room reserved in the struct DIR itself like in "modern" implementations), and these systems provide readdir_r() for the "thread-safe" way. I suppose such system wouldn't emit a warning for readdir_r(), my patch addresses them while yours does not. So the question is, does such system exists and do we want to still support it?
Bug report for APR [2019/03/31]
+---+ | Bugzilla Bug ID | | +-+ | | Status: UNC=Unconfirmed NEW=New ASS=Assigned| | | OPN=ReopenedVER=Verified(Skipped Closed/Resolved) | | | +-+ | | | Severity: BLK=Blocker CRI=Critical REG=Regression MAJ=Major | | | | MIN=Minor NOR=NormalENH=Enhancement TRV=Trivial | | | | +-+ | | | | Date Posted | | | | | +--+ | | | | | Description | | | | | | | |16056|Inf|Enh|2003-01-14|Shared memory & mutex ownership not correctly esta| |20382|New|Nor|2003-05-31|Poor performance on W2000 AS | |28453|New|Enh|2004-04-18|apr_uri should parse relative to a base URI | |33188|Inf|Nor|2005-01-21|libtool linking failure on Suse | |33490|Inf|Nor|2005-02-10|APR does not compile with Borland C++ | |38410|New|Nor|2006-01-27|apr/win32 misinterpreted the meaning of WAIT_ABAND| |39289|New|Enh|2006-04-12|test suite additions for trylock functions| |39853|Inf|Nor|2006-06-21|apr_strtoi64 does not build on WinCE due to lack o| |39895|New|Enh|2006-06-23|apr_os_strerror on WinCE needs to xlate unicode->u| |39896|New|Enh|2006-06-23|Output test status to OutputDebugString in additio| |40020|New|Enh|2006-07-11|Add support for apr_uint8_t and apr_int8_t types | |40193|Inf|Nor|2006-08-06|Patches to support different compiler than EMX on | |40622|New|Enh|2006-09-27|enhance apr temp files on NT to be more secure| |40758|Ver|Maj|2006-10-15|WIN64, apr_vformatter(..) cannot handle 64bit poin| |40939|New|Enh|2006-11-09|pool minimal allocation size should be configurabl| |41192|Inf|Trv|2006-12-17|Add the expat libtool file to the LT_LDFLAGS varia| |41254|New|Enh|2006-12-28|apr_queue_t enhancements | |41351|Inf|Enh|2007-01-11|Tivoli LDAP SDK support in aprutil| |41352|Inf|Min|2007-01-11|openldap and per-connection client certificates in| |41916|Inf|Nor|2007-03-21|MinGW cross-compile support for Linux | |42365|New|Enh|2007-05-09|Suppress console for apr_proc_create() created pro| |42682|New|Maj|2007-06-17|Apache child terminates with signal 11 when using | |42728|New|Nor|2007-06-23|mod_ssl thread detaching not releasing handles| |42848|New|Enh|2007-07-10|add IP TOS support to apr_socket_opt_set()| |43035|New|Enh|2007-08-04|Add ability to wrap ssl around pre-existing socket| |43066|New|Nor|2007-08-08|get_password on Windows is kludgy | |43152|Inf|Nor|2007-08-16|apr_socket_opt_get doesn't work with APR_SO_SNDBUF| |43172|Ass|Nor|2007-08-20|apr-util don't want to find mozldap-6.x | |43217|New|Min|2007-08-26|All-ones IPv6 subnet mask not accepted| |43244|New|Enh|2007-08-29|apr_socket_t missing dup, dup2 and setaside | |43302|New|Nor|2007-09-04|apr_bucket_socket doesn't work with non-connected | |43309|New|Enh|2007-09-05|add apr_socket_sendtov support| |43375|New|Nor|2007-09-13|Pool integrity check fails for apr threads| |43499|New|Nor|2007-09-27|apr_global_mutex_create returns error "Access deni| |43507|New|Enh|2007-09-28|configure flag for SHELL_PATH | |43508|New|Enh|2007-09-28|Please be available whether atomics use thread mut| |43793|New|Enh|2007-11-04|builtin atomics on Windows| |44127|New|Enh|2007-12-21|File Extended Attributes Support | |44128|New|Enh|2007-12-21|apr_os_file_put() does not register a cleanup hand| |44129|New|Enh|2007-12-21|apr_os_dir_put() does not allocate an entry buffer| |44186|New|Nor|2008-01-08|[PATCH] Add memcached 1.2.4 features to apr_memcac| |44230|New|Enh|2008-01-14|Add Ark Linux support to config.layout| |44432|New|Enh|2008-02-15|patch - proposal for application function hooks | |44550|Inf|Maj|2008-03-06|Solaris sendfilev() handling - EINTR appears to ha| |45251|New|Nor|2008-06-22|DBD MySQL driver doesn't support multiple resultse| |45291|New|Nor|2008-06-26|apr_thread_t is leaking | |45298|New|Maj|2008-06-27|apr_os_thread_get() differs between windows and un| |45407|Opn|Nor|2008-07-16|auto reconnect in apr_dbd_mysql disturb normal wor| |45455|New|Nor|2008-07-22|rwlock sometimes allows a writer to take the lock | |45494|Opn|Nor|2008-07-28|testsockets fails on Solaris when IPv6 interfaces | |45496|New|Enh|2008-07-29|[patch] adding directory matching [dir/**/conf.d/*|
