httpd-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From William A Rowe Jr <wr...@rowe-clan.net>
Subject Re: Balancers (file based SHMs) and restart issue (Windows only?)
Date Wed, 09 Sep 2015 16:08:19 GMT
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 <ylavic.dev@gmail.com> 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 <jim@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.dev@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?
> >
>

Mime
View raw message