Re: Fwd: FYI: change to travis-ci emailer could cause moderation headaches
Le 26/01/2022 à 14:58, Eric Covener a écrit : I noticed I stopped getting "Travis CI" emails for httpd around 10/21. But I see people still discussing CI failures, so I am a little confused. Maybe they are only seeing it in the context of PRs. For me, manual check of https://app.travis-ci.com/github/apache/httpd/builds as well. CJ Did we lose notifications to dev@ as a result of the below? Or something else? -- Forwarded message - From: sebb Date: Thu, Oct 28, 2021 at 11:08 AM Subject: FYI: change to travis-ci emailer could cause moderation headaches To: Users It looks like mails from bui...@travis-ci.com are now using a different email provider. This means that the envelope sender has changed. They used to use mandrillapp.com, but now seem to use amazonses.com mandrillapp.com used a dynamic envelope sender, but there was a pattern to it which meant that the underlying sender could be detected (see INFRA-18843) It's not clear if the amazonses.com envelopes have a common pattern that could be leveraged in the same way. If not, I suspect every email from travis.com will need to be moderated. Unless someone has a better idea of how to allow such emails without opening the floodgates. Sebb
Re: mpm event assertion failures
On 1/27/22 9:57 AM, Ruediger Pluem wrote: > > > On 1/26/22 6:34 PM, Yann Ylavic wrote: >> On Wed, Jan 26, 2022 at 5:07 PM Graham Leggett wrote: >>> >>> We need to clarify expectations at this point. >>> >>> The trunk of httpd has a policy called “commit then review” (CTR), meaning >>> that changes are applied first, and then review is done to see what the >>> ramifications of those changes are. Some changes are at a high level and >>> very well contained, some changes such as this one are at a very low level >>> and affect the whole server. Obviously there is an expectation that one >>> must think it works before committing, but none of our contributors have >>> access to even a fraction of the number of platforms that httpd runs on, >>> and so we must rely on both our CI and the review of others (thus the “then >>> review”) to show us where things have gone wrong. Our CI is a tool to tell >>> us what potentially has gone wrong across a wide set of scenarios, far >>> beyond the capability of what a single person has access to. >> >> I agree with all of the above, there is nothing wrong with the way you did >> it. >> Maybe now that we have a better ci that runs on each branch, a github >> PR could be created first to see/test the results before checking in >> to trunk (this was not an option a few years ago)? >> This still allows for review and/or help if something goes wrong (by >> asking on dev@ if needed), while others don't have to wait for trunk >> to work for their own changes. > > This sounds like an excellent proposal here to me as it looks like that > finding the issue takes longer and it removes stress from > everyone. There are some tools in stock that will help you with this. Once you reverted the revisons on trunk, you can take these revisions and use a modified version of https://svn.apache.org/repos/asf/httpd/dev-tools/github/create_pr.sh . Basically you need to replace 2.4.x with trunk and override BRANCH_NAME near the top of the script with the name of your feature branch branched from the reverted trunk. Once the PR is ready to go into to trunk you can leave it to https://svn.apache.org/repos/asf/httpd/dev-tools/github/apply_trunk_pr.sh to apply this PR to your svn working copy of trunk with the commit message ready in the clog file for your svn commit. Regards Rüdiger
Re: mpm event assertion failures
Testing my changes, I noticed that the poxy/ssl tests now take about 4 minutes to complete. I reverted and it stays the same on current trunk: trunk (r81897548), unchanged: t/ssl/proxy.t .. ok All tests successful. Files=1, Tests=290, 249 wallclock secs ( 0.12 usr 0.01 sys + 0.73 cusr 0.16 csys = 1.02 CPU) Result: PASS Before the handshake changes, it runs in 9 seconds: trunk (r1897273) t/ssl/proxy.t .. ok All tests successful. Files=1, Tests=290, 9 wallclock secs ( 0.12 usr 0.01 sys + 0.72 cusr 0.16 csys = 1.01 CPU) Result: PASS How should we proceed with this? Shall I setup a branch with the changes by Graham, so we can inspect it and discuss via github? Graham, is that ok with you? I am open to other ideas. Kind Regards, Stefan > Am 27.01.2022 um 10:50 schrieb Stefan Eissing : > > > >> Am 26.01.2022 um 17:06 schrieb Graham Leggett : >> >> On 26 Jan 2022, at 13:49, Stefan Eissing wrote: >> >>> Guys, we have changes in a central part of the server and our CI fails.=20= >>> >>> It is not good enough to give other people unspecific homework to fix = >>> it.=20 >>> >>> Analyze what you broke and if we can help, we'll happily do that. But >>> you need to give some more details here. >> >> We need to clarify expectations at this point. >> >> The trunk of httpd has a policy called “commit then review” (CTR), meaning >> that changes are applied first, and then review is done to see what the >> ramifications of those changes are. Some changes are at a high level and >> very well contained, some changes such as this one are at a very low level >> and affect the whole server. Obviously there is an expectation that one must >> think it works before committing, but none of our contributors have access >> to even a fraction of the number of platforms that httpd runs on, and so we >> must rely on both our CI and the review of others (thus the “then review”) >> to show us where things have gone wrong. Our CI is a tool to tell us what >> potentially has gone wrong across a wide set of scenarios, far beyond the >> capability of what a single person has access to. >> >> The next issue is “Analyze what you broke”. >> >> I have been working on this code morning, day and night for many days now. A >> lot of time was spent chasing what I thought was an infinite loop >> complaining about EOF, but actually was a harmless error that should have >> been a debug triggered every time the client disconnected. Then more time >> was spent trying to get to the bottom of why the timeouts weren’t working, >> only to discover that the timeouts weren’t implemented. The accusation that >> I have been careless is highly offensive. >> >> In open source we don’t bark accusations at each other, particularly when >> noone has seen just how much time and effort has gone into this. I have been >> working on this code for 25 years and am not afraid to call this out, but >> new people to open source or new to this project are going to be intimidated >> and leave. This must not happen. >> >> Please be mindful of others working on this project, and be helpful where >> possible. > > I did not intend to "bark" at you and am sorry if my reply came across like > that. > > As you explained, this change has been very taxing and a struggle. We did not > see that. What we experience are changes that prevent us from working in > trunk. If you, at that point in time, are unable to help because > workload/energy/or any other issues that is understood. This is a volunteer > project. But understand that others are in the same situation as you and > experience the changes as a showstopper. > > > As Yann pointed out much more constructively than myself, isolating such a > change in a PR where the team can review, discuss and enhance it together > seems the best approach. This does not mean every commit needs to be a PR. It > can be done retroactively by reverting breaking changes and place them in a > PR to work together to make it good. > > Let's do this and see how it works. > > Kind Regards, > Stefan
Re: svn commit: r1897472 - in /httpd/httpd/trunk: include/httpd.h server/mpm/event/event.c server/mpm/prefork/prefork.c server/mpm/winnt/child.c server/mpm/worker/worker.c server/util.c
On Thu, Jan 27, 2022 at 9:40 AM Ruediger Pluem wrote:
>
> On 1/25/22 9:28 PM, yla...@apache.org wrote:
> >
> > -#if AP_HAS_THREAD_LOCAL
> > -/* Create an apr_thread_t for the main child thread to set up its
> > - * Thread Local Storage. Since it's detached and it won't
> > - * apr_thread_exit(), destroy its pool before exiting via
> > - * a pchild cleanup
> > - */
> > -{
> > -apr_thread_t *main_thd = NULL;
> > -apr_threadattr_t *main_thd_attr = NULL;
> > -if (apr_threadattr_create(_thd_attr, pchild)
> > -|| apr_threadattr_detach_set(main_thd_attr, 1)
> > -|| ap_thread_current_create(_thd, main_thd_attr,
> > -pchild)) {
> > -ap_log_error(APLOG_MARK, APLOG_EMERG, 0, ap_server_conf,
> > APLOGNO(10376)
> > - "Couldn't initialize child main thread");
> > -exit(APEXIT_CHILDINIT);
> > -}
> > -apr_pool_cleanup_register(pchild, main_thd, main_thread_cleanup,
> > - apr_pool_cleanup_null);
> > -}
> > -#endif
> > -
>
> Why is this no longer needed?
Because Windows children processes run the main() too, so
ap_thread_main_create() has been called already.
Regards;
Yann.
Re: svn commit: r1897460 - in /httpd/httpd/trunk: include/ modules/core/ modules/http2/ modules/ssl/ server/ server/mpm/event/ server/mpm/prefork/ server/mpm/winnt/ server/mpm/worker/
On Thu, Jan 27, 2022 at 9:36 AM Ruediger Pluem wrote: > > Would it make sense to move the above code to mpm_common.c as it looks very > similar for each MPM? Good point, in r1897543 I replaced ap_thread_current_create() by ap_thread_main_create() which is how it's used in httpd anyway. That's still in util.c because it's used by main() too, one more common code ;) Regards; Yann.
Re: mpm event assertion failures
> Am 26.01.2022 um 17:06 schrieb Graham Leggett : > > On 26 Jan 2022, at 13:49, Stefan Eissing wrote: > >> Guys, we have changes in a central part of the server and our CI fails.=20= >> >> It is not good enough to give other people unspecific homework to fix = >> it.=20 >> >> Analyze what you broke and if we can help, we'll happily do that. But >> you need to give some more details here. > > We need to clarify expectations at this point. > > The trunk of httpd has a policy called “commit then review” (CTR), meaning > that changes are applied first, and then review is done to see what the > ramifications of those changes are. Some changes are at a high level and very > well contained, some changes such as this one are at a very low level and > affect the whole server. Obviously there is an expectation that one must > think it works before committing, but none of our contributors have access to > even a fraction of the number of platforms that httpd runs on, and so we must > rely on both our CI and the review of others (thus the “then review”) to show > us where things have gone wrong. Our CI is a tool to tell us what potentially > has gone wrong across a wide set of scenarios, far beyond the capability of > what a single person has access to. > > The next issue is “Analyze what you broke”. > > I have been working on this code morning, day and night for many days now. A > lot of time was spent chasing what I thought was an infinite loop complaining > about EOF, but actually was a harmless error that should have been a debug > triggered every time the client disconnected. Then more time was spent trying > to get to the bottom of why the timeouts weren’t working, only to discover > that the timeouts weren’t implemented. The accusation that I have been > careless is highly offensive. > > In open source we don’t bark accusations at each other, particularly when > noone has seen just how much time and effort has gone into this. I have been > working on this code for 25 years and am not afraid to call this out, but new > people to open source or new to this project are going to be intimidated and > leave. This must not happen. > > Please be mindful of others working on this project, and be helpful where > possible. I did not intend to "bark" at you and am sorry if my reply came across like that. As you explained, this change has been very taxing and a struggle. We did not see that. What we experience are changes that prevent us from working in trunk. If you, at that point in time, are unable to help because workload/energy/or any other issues that is understood. This is a volunteer project. But understand that others are in the same situation as you and experience the changes as a showstopper. As Yann pointed out much more constructively than myself, isolating such a change in a PR where the team can review, discuss and enhance it together seems the best approach. This does not mean every commit needs to be a PR. It can be done retroactively by reverting breaking changes and place them in a PR to work together to make it good. Let's do this and see how it works. Kind Regards, Stefan
Re: mpm event assertion failures
On 1/26/22 6:34 PM, Yann Ylavic wrote: > On Wed, Jan 26, 2022 at 5:07 PM Graham Leggett wrote: >> >> We need to clarify expectations at this point. >> >> The trunk of httpd has a policy called “commit then review” (CTR), meaning >> that changes are applied first, and then review is done to see what the >> ramifications of those changes are. Some changes are at a high level and >> very well contained, some changes such as this one are at a very low level >> and affect the whole server. Obviously there is an expectation that one must >> think it works before committing, but none of our contributors have access >> to even a fraction of the number of platforms that httpd runs on, and so we >> must rely on both our CI and the review of others (thus the “then review”) >> to show us where things have gone wrong. Our CI is a tool to tell us what >> potentially has gone wrong across a wide set of scenarios, far beyond the >> capability of what a single person has access to. > > I agree with all of the above, there is nothing wrong with the way you did it. > Maybe now that we have a better ci that runs on each branch, a github > PR could be created first to see/test the results before checking in > to trunk (this was not an option a few years ago)? > This still allows for review and/or help if something goes wrong (by > asking on dev@ if needed), while others don't have to wait for trunk > to work for their own changes. This sounds like an excellent proposal here to me as it looks like that finding the issue takes longer and it removes stress from everyone. Regards Rüdiger
Re: svn commit: r1897472 - in /httpd/httpd/trunk: include/httpd.h server/mpm/event/event.c server/mpm/prefork/prefork.c server/mpm/winnt/child.c server/mpm/worker/worker.c server/util.c
On 1/25/22 9:28 PM, yla...@apache.org wrote:
> Author: ylavic
> Date: Tue Jan 25 20:28:28 2022
> New Revision: 1897472
>
> URL: http://svn.apache.org/viewvc?rev=1897472=rev
> Log:
> core: Follow up to r1897460: Implement and use ap_thread_current_after_fork().
>
> thread_local variables are not (always?) reset on fork(), so we need a way
> to set the current_thread to NULL in the child process.
>
> Implement and use ap_thread_current_after_fork() for that.
>
> * include/httpd.h:
> Define ap_thread_current_after_fork().
>
> * server/util.c:
> Implement ap_thread_current_after_fork().
>
> * server/mpm/event/event.c, server/mpm/prefork/prefork.c,
> server/mpm/worker/worker.c:
> Use ap_thread_current_after_fork().
>
> * server/mpm/winnt/child.c:
> Windows processes are not fork()ed and each child runs the main(), so
> ap_thread_current_create() was already called there.
>
>
> Modified:
> httpd/httpd/trunk/include/httpd.h
> httpd/httpd/trunk/server/mpm/event/event.c
> httpd/httpd/trunk/server/mpm/prefork/prefork.c
> httpd/httpd/trunk/server/mpm/winnt/child.c
> httpd/httpd/trunk/server/mpm/worker/worker.c
> httpd/httpd/trunk/server/util.c
>
> Modified: httpd/httpd/trunk/server/mpm/winnt/child.c
> URL:
> http://svn.apache.org/viewvc/httpd/httpd/trunk/server/mpm/winnt/child.c?rev=1897472=1897471=1897472=diff
> ==
> --- httpd/httpd/trunk/server/mpm/winnt/child.c (original)
> +++ httpd/httpd/trunk/server/mpm/winnt/child.c Tue Jan 25 20:28:28 2022
> @@ -891,15 +891,6 @@ static void create_listener_thread(void)
> }
>
>
> -#if AP_HAS_THREAD_LOCAL
> -static apr_status_t main_thread_cleanup(void *arg)
> -{
> -apr_thread_t *thd = arg;
> -apr_pool_destroy(apr_thread_pool_get(thd));
> -return APR_SUCCESS;
> -}
> -#endif
> -
> void child_main(apr_pool_t *pconf, DWORD parent_pid)
> {
> apr_status_t status;
> @@ -921,28 +912,6 @@ void child_main(apr_pool_t *pconf, DWORD
> apr_pool_create(, pconf);
> apr_pool_tag(pchild, "pchild");
>
> -#if AP_HAS_THREAD_LOCAL
> -/* Create an apr_thread_t for the main child thread to set up its
> - * Thread Local Storage. Since it's detached and it won't
> - * apr_thread_exit(), destroy its pool before exiting via
> - * a pchild cleanup
> - */
> -{
> -apr_thread_t *main_thd = NULL;
> -apr_threadattr_t *main_thd_attr = NULL;
> -if (apr_threadattr_create(_thd_attr, pchild)
> -|| apr_threadattr_detach_set(main_thd_attr, 1)
> -|| ap_thread_current_create(_thd, main_thd_attr,
> -pchild)) {
> -ap_log_error(APLOG_MARK, APLOG_EMERG, 0, ap_server_conf,
> APLOGNO(10376)
> - "Couldn't initialize child main thread");
> -exit(APEXIT_CHILDINIT);
> -}
> -apr_pool_cleanup_register(pchild, main_thd, main_thread_cleanup,
> - apr_pool_cleanup_null);
> -}
> -#endif
> -
> ap_run_child_init(pchild, ap_server_conf);
>
> listener_shutdown_event = CreateEvent(NULL, TRUE, FALSE, NULL);
>
Why is this no longer needed?
Regards
Rüdiger
Re: svn commit: r1897460 - in /httpd/httpd/trunk: include/ modules/core/ modules/http2/ modules/ssl/ server/ server/mpm/event/ server/mpm/prefork/ server/mpm/winnt/ server/mpm/worker/
On 1/25/22 6:34 PM, yla...@apache.org wrote:
> Author: ylavic
> Date: Tue Jan 25 17:34:57 2022
> New Revision: 1897460
>
> URL: http://svn.apache.org/viewvc?rev=1897460=rev
> Log:
> core: Efficient ap_thread_current() when apr_thread_local() is missing.
>
> #define ap_thread_create, ap_thread_current_create and ap_thread_current to
> their apr-1.8+ equivalent if available, or implement them using the compiler's
> thread_local mechanism if available, or finally provide stubs otherwise.
>
> #define AP_HAS_THREAD_LOCAL to 1 in the two former case or 0 otherwise, while
> AP_THREAD_LOCAL is defined to the compiler's keyword iff AP_HAS_THREAD_LOCAL.
>
> Replace all apr_thread_create() calls with ap_thread_create() so that httpd
> threads can use ap_thread_current()'s pool data as Thread Local Storage.
>
> Bump MMN minor.
>
> * include/httpd.h():
> Define AP_HAS_THREAD_LOCAL, AP_THREAD_LOCAL (eventually),
> ap_thread_create(),
> ap_thread_current_create() and ap_thread_current().
>
> * server/util.c:
> Implement ap_thread_create(), ap_thread_current_create() and
> ap_thread_current() when APR < 1.8.
>
> * modules/core/mod_watchdog.c, modules/http2/h2_workers.c,
> modules/ssl/mod_ssl_ct.c:
> Use ap_thread_create() instead of apr_thread_create.
>
> * server/main.c:
> Use AP_HAS_THREAD_LOCAL and ap_thread_current_create instead of APR's.
>
> * server/util_pcre.c:
> Use AP_HAS_THREAD_LOCAL and ap_thread_current instead of APR's.
>
> * server/mpm/event/event.c, server/mpm/worker/worker.c,
> server/mpm/prefork/prefork.c:
> Use ap_thread_create() instead of apr_thread_create.
> Create an apr_thread_t/ap_thread_current() for the main chaild thread usable
> at child_init().
>
> * server/mpm/winnt/child.c:
> Use ap_thread_create() instead of CreateThread().
> Create an apr_thread_t/ap_thread_current() for the main chaild thread usable
>
>
> Modified:
> httpd/httpd/trunk/include/ap_mmn.h
> httpd/httpd/trunk/include/httpd.h
> httpd/httpd/trunk/modules/core/mod_watchdog.c
> httpd/httpd/trunk/modules/http2/h2_workers.c
> httpd/httpd/trunk/modules/ssl/mod_ssl_ct.c
> httpd/httpd/trunk/server/main.c
> httpd/httpd/trunk/server/mpm/event/event.c
> httpd/httpd/trunk/server/mpm/prefork/prefork.c
> httpd/httpd/trunk/server/mpm/winnt/child.c
> httpd/httpd/trunk/server/mpm/worker/worker.c
> httpd/httpd/trunk/server/util.c
> httpd/httpd/trunk/server/util_pcre.c
>
>
> Modified: httpd/httpd/trunk/server/mpm/event/event.c
> URL:
> http://svn.apache.org/viewvc/httpd/httpd/trunk/server/mpm/event/event.c?rev=1897460=1897459=1897460=diff
> ==
> --- httpd/httpd/trunk/server/mpm/event/event.c (original)
> +++ httpd/httpd/trunk/server/mpm/event/event.c Tue Jan 25 17:34:57 2022
> @@ -2880,6 +2880,15 @@ static void join_start_thread(apr_thread
> }
> }
>
> +#if AP_HAS_THREAD_LOCAL
> +static apr_status_t main_thread_cleanup(void *arg)
> +{
> +apr_thread_t *thd = arg;
> +apr_pool_destroy(apr_thread_pool_get(thd));
> +return APR_SUCCESS;
> +}
> +#endif
> +
> static void child_main(int child_num_arg, int child_bucket)
> {
> apr_thread_t **threads;
> @@ -2902,6 +2911,28 @@ static void child_main(int child_num_arg
> apr_pool_create(, pconf);
> apr_pool_tag(pchild, "pchild");
>
> +#if AP_HAS_THREAD_LOCAL
> +/* Create an apr_thread_t for the main child thread to set up its
> + * Thread Local Storage. Since it's detached and it won't
> + * apr_thread_exit(), destroy its pool before exiting via
> + * a pchild cleanup
> + */
> +if (!one_process) {
> +apr_thread_t *main_thd = NULL;
> +apr_threadattr_t *main_thd_attr = NULL;
> +if (apr_threadattr_create(_thd_attr, pchild)
> +|| apr_threadattr_detach_set(main_thd_attr, 1)
> +|| ap_thread_current_create(_thd, main_thd_attr,
> +pchild)) {
> +ap_log_error(APLOG_MARK, APLOG_EMERG, 0, ap_server_conf,
> APLOGNO()
> + "Couldn't initialize child main thread");
> +clean_child_exit(APEXIT_CHILDFATAL);
> +}
> +apr_pool_cleanup_register(pchild, main_thd, main_thread_cleanup,
> + apr_pool_cleanup_null);
> +}
> +#endif
> +
> /* close unused listeners and pods */
> for (i = 0; i < retained->mpm->num_buckets; i++) {
> if (i != child_bucket) {
Would it make sense to move the above code to mpm_common.c as it looks very
similar for each MPM?
Regards
Rüdiger
