That looks like a good solution. FWIW, ap_mpm_query(AP_MPMQ_IS_FORKED... would be another way to express whether we need the config_gen.
On Tue, Sep 8, 2015 at 11:33 AM, Yann Ylavic <[email protected]> wrote: > 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 <[email protected]> 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 <[email protected]> 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? > > >
