On Tue, May 8, 2018 at 7:19 PM, Jim Jagielski <j...@jagunet.com> wrote:
> I am under the impression is that we should likely restore mod_slotmem_shm
> back to its "orig" condition,

So I did this (r1831868),

> either:
>
>    o
> http://svn.apache.org/viewvc/httpd/httpd/trunk/modules/slotmem/mod_slotmem_shm.c?view=markup&pathrev=1822341

and this too (r1831869).

>
> Just for the heck of it, didn't r1822341 actually *FIX* the PR?

Not enough to pass all the tests raised by both PRs 62044 and 62308,
hence follow ups r1831870, r1831871+r1831935 and finally
r1831938.

I think I owe an explanation on to why I made those changes in
mod_slotmem_shm for 2.4.33, and why until now it looked to me like the
right thing to do.

Initially I thought that, on unixes MPMs, mod_slotmem_shm was
reusing/preserving SHMs on restart (some parts of the code were quite
misleading in this regard), and thus switching to per generation SHMs
(like on Windows) was potentially going to break Unix users.
Actually we never have reused SHMs on restart on any system unless
BalancerPersist is enabled, while for me BalancerPersist was meant to
preserve data on stop/start only.
In any case this led me to take the "SHMs maintained in pglobal"
approach for all OSes (creating new ones only when sizes change), as I
thought it would allow to preserve SHMs on Windows too (IOW, was meant
to fix Windows rather than break Unix).
This has been my reasoning from the start, I only cared about fixing
the reported bugs and preserving the existing behaviour (supposedly),
not really an irrepressible desire from me to change/refactor the code
(as you have suggested several times).

Anyway, this was before I started to work on the last issue reported
in PR 62308 (change some BalancerMember name/port and httpd won't
restart), where I realized that sharing SHMs between old and new
generations can't work in all cases (at least without more non-small
changes), even when the sizes don't change. So I wondered if this
particular case worked in 2.4.29, and found that SHMs were re-created
each time, so it worked.. until BalancerPersist was enabled (same
failure).

So you were right about the potential to break things with my changes
in the code, though it didn't work as expected already.
Btw, I would have preferred more constructive feedbacks, including in
the discussions about PR 62044 and the original r1822341 commit thread
(where I explained why I was going to revert it..).

Now we are back to 2.4.29 code, r1822341 is in again, and I committed
additional changes (minimal hopefully) to address the issues reported
in PR 62308 (and PR 62044 still). My own testing, based on the tests
run by the OP (his on Windows, mine on Linux), all passes right now.
So I'm waiting for the OP's latest results to propose a backport.

WDYT of this approach (and patches), do it sound better?

Regards,
Yann.

Reply via email to