Thanks, Yann; This does help explain the rationale and I appreciate you taking the time to walk us through the reasoning.
-- Daniel Ruggeri On 5/20/2018 3:59 PM, Yann Ylavic wrote: > 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.