I didn't reproduce it myself (no Windows machine), but the scenario
and traces in PR 58024 show what happens quite clearly.
On (graceful) restart, if one or more children remain when pconf is
cleared (SHMs are destroyed), apr_file_remove() fails (open file =>
ERROR_ACCESS_DENIED until all the HANDLEs to the file are closed,
whereafter the file is removed from the filesystem).
Since the files exist, the next slotmem_create() will attach (and not
recreate) them, which fails when some balancers/members are added
(size check).
I agree this does not concern Unixes (which use a different inode for
each new file, be there an opened fd on the previous inode's -same-
filename or not).
So the fix could be quite simple, like:
Index: modules/slotmem/mod_slotmem_shm.c
===================================================================
--- modules/slotmem/mod_slotmem_shm.c (revision 1701559)
+++ modules/slotmem/mod_slotmem_shm.c (working copy)
@@ -103,10 +103,29 @@ static const char *slotmem_filename(apr_pool_t *po
return NULL;
}
else if (slotmemname[0] != '/') {
- const char *filenm = apr_pstrcat(pool, DEFAULT_SLOTMEM_PREFIX,
- slotmemname, DEFAULT_SLOTMEM_SUFFIX,
- NULL);
- fname = ap_runtime_dir_relative(pool, filenm);
+ fname = slotmemname;
+#ifdef WIN32
+ /* On Windows, apr_file_remove() (i.e. DeleFile()) marks the file for
+ * deletion on close, hence not before the last HANDLE to it is closed.
+ * Thus on graceful restart (the only restart mode with mpm_winnt), the
+ * old file may still exist until all the children stop, while we ought
+ * to create a new one for our new clear SHM. Therefore, we would only
+ * be able to reuse (attach) the old SHM, preventing some changes to
+ * the config file, like the number of balancers/members, since the
+ * size check (to fit the new config) would fail. Let's avoid this by
+ * including the generation number in the SHM filename (obviously not
+ * the persisted name!), which pretty much acts like files on Unixes
+ * where the OS maintains inodes which can be unlink()ed while opened,
+ * when new files with the same name will reference a new inode.
+ */
+ if (!persist) {
+ fname = apr_psprintf(pool, "%s_%x", fname,
+ ap_state_query(AP_SQ_CONFIG_GEN));
+ }
+#endif
+ fname = apr_pstrcat(pool, DEFAULT_SLOTMEM_PREFIX, fname,
+ DEFAULT_SLOTMEM_SUFFIX, NULL);
+ fname = ap_runtime_dir_relative(pool, fname);
}
else {
fname = slotmemname;
--
If this is OK, I'll link it to PR 58024 for the OP to give it a chance
(since I can't myself)...
WDYT?
On Fri, Sep 4, 2015 at 1:01 PM, Jim Jagielski <j...@jagunet.com> wrote:
> Instead of changing the default for all OSs, sounds like
> some Windows specific changes may need to be done. Has anyone
> using Windows actually seen this or confirmed this is
> possible??
>
>> On Sep 2, 2015, at 5:08 AM, Yann Ylavic <ylavic....@gmail.com> wrote:
>>
>> Re PR 58024.
>>
>> AIUI, balancers (and members) SHM slots are destroyed (and the
>> underlying base file removed) with pconf before restarting and then
>> re-created after (according to the new configuration, eg. number of
>> balancers/members, be it the same or not).
>> Persisted slots are saved in their own file (with the .persit suffix),
>> and reused (copied) only if the configuration did not change in
>> between.
>> (@Jim, I see now how this is better than attaching ;)
>>
>> This works well on Unixes, but on Windows files can't be removed
>> (unlinked) while any process/thread hold an HANDLE on it, hence until
>> all the children have exited...
>> Thus, since the SHM files still exist on restart, the balancer code
>> tries to attach them instead of re-creating, and issues the checks on
>> the existing size to fit the reloaded configuration, and fail should
>> should any balancer/member be added or removed => PR 58024.
>>
>> BTW, even on Unixes there is a race on these files between the main
>> process and the children, or the children themselves from different
>> "generation".
>>
>> So I wonder if we could use anonymous SHMs instead (or mktemp based
>> when not native), persisted slots are saved in their own files anyway
>> and won't be affected.
>> Otherwise, maybe we could use the "generation" as part of the file
>> name, but that's equivalent and more complex IMO.
>>
>> Thoughts?
>
