httpd-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Yann Ylavic <ylavic....@gmail.com>
Subject Re: Balancers (file based SHMs) and restart issue (Windows only?)
Date Mon, 14 Sep 2015 13:45:06 GMT
PR 58024 looks hard(er) to resolve (details there)...

It seems that DeleteFile() always fails when some file mapping exists
on the file, which also prevents "deletion when the last handle to the
file is closed".
So currently (mod_slotmem_shm on trunk), the SHM files by generation
leak on the filesystem since they are never removed (successfully).

The only thing I can think of now is to use FILE_FLAG_DELETE_ON_CLOSE
flagged files, but it's not available for SHM files in APR, so we'd
have to cook our own slotmem_shm_create() (a wrapper with Windows
specific code) which would apr_os_shm_put() a native SHM from a
apr_file_open(..., APR_FOPEN_DELONCLOSE) file.

Any better idea?

On Wed, Sep 9, 2015 at 6:08 PM, William A Rowe Jr <wrowe@rowe-clan.net> wrote:
> 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