Yann, thanks for your perseverance on this.

Could you, just as a rough description, list which
test cases would have prevented the bugs? Maybe someone
would feel like implementing them (or in case of a future
code change there, could at least manually find some
instructions on what to test in the mailing list archive).

E.g.
- configure slotmem as 1) XYZ, 2) ABC with persistence, 3) DEF...
- start, request something, expect bla1
- stop+start request another thing, expect bla2
- graceful, request, expect bla3

Just while it is fresh in your mind... 

Cheers,

Stefan

> Am 20.05.2018 um 22:59 schrieb Yann Ylavic <ylavic....@gmail.com>:
> 
> 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