You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@httpd.apache.org by GitBox <gi...@apache.org> on 2022/03/16 12:49:00 UTC
[GitHub] [httpd] icing opened a new pull request #297: H2 pollset impr
icing opened a new pull request #297:
URL: https://github.com/apache/httpd/pull/297
* `core`: add hook `create_secondary_connection` and method to core, since the implementation details for those connections to a master one belong there.
* `mod_http2`: use new hook and also recycle transit pools/bucket_alloc for those connections.
* `mod_http2`: improvements in pollset handling for secondary connections.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@httpd.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@httpd.apache.org
For additional commands, e-mail: notifications-help@httpd.apache.org
[GitHub] [httpd] icing commented on pull request #297: H2 pollset impr
Posted by GitBox <gi...@apache.org>.
icing commented on pull request #297:
URL: https://github.com/apache/httpd/pull/297#issuecomment-1072249969
Merged as r1899032 in trunk. Thanks for reviewing!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@httpd.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@httpd.apache.org
For additional commands, e-mail: notifications-help@httpd.apache.org
[GitHub] [httpd] icing commented on a change in pull request #297: H2 pollset impr
Posted by GitBox <gi...@apache.org>.
icing commented on a change in pull request #297:
URL: https://github.com/apache/httpd/pull/297#discussion_r829055181
##########
File path: modules/http2/h2_mplx.c
##########
@@ -254,18 +314,20 @@ h2_mplx *h2_mplx_c1_create(h2_stream *stream0, server_rec *s, apr_pool_t *parent
m->streams_ev_in = apr_array_make(m->pool, 10, sizeof(h2_stream*));
m->streams_ev_out = apr_array_make(m->pool, 10, sizeof(h2_stream*));
-#if !H2_POLL_STREAMS
+ m->streams_input_read = h2_iq_create(m->pool, 10);
status = apr_thread_mutex_create(&m->poll_lock, APR_THREAD_MUTEX_DEFAULT,
m->pool);
if (APR_SUCCESS != status) goto failure;
- m->streams_input_read = h2_iq_create(m->pool, 10);
+#if !H2_POLL_STREAMS
m->streams_output_written = h2_iq_create(m->pool, 10);
#endif
conn_ctx = h2_conn_ctx_get(m->c1);
mplx_pollset_add(m, conn_ctx);
m->scratch_r = apr_pcalloc(m->pool, sizeof(*m->scratch_r));
+ m->max_spare_transits = 3;
Review comment:
This was a start setting and I plan to continue work on this.
Atm I am considering a global reserve list, because when connection go idle, this just hangs around useless.
The global list should probably keep not more than H2MaxWorker transits around.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@httpd.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@httpd.apache.org
For additional commands, e-mail: notifications-help@httpd.apache.org
[GitHub] [httpd] icing commented on a change in pull request #297: H2 pollset impr
Posted by GitBox <gi...@apache.org>.
icing commented on a change in pull request #297:
URL: https://github.com/apache/httpd/pull/297#discussion_r829051246
##########
File path: modules/http2/h2_conn_ctx.h
##########
@@ -48,20 +50,17 @@ struct h2_conn_ctx_t {
struct h2_bucket_beam *beam_out; /* c2: data out, created from req_pool */
struct h2_bucket_beam *beam_in; /* c2: data in or NULL, borrowed from request stream */
- apr_pool_t *mplx_pool; /* c2: an mplx child pool for safe use inside mplx lock */
apr_file_t *pipe_in_prod[2]; /* c2: input produced notification pipe */
- apr_file_t *pipe_in_drain[2]; /* c2: input drained notification pipe */
apr_file_t *pipe_out_prod[2]; /* c2: output produced notification pipe */
- apr_pollfd_t pfd_in_drain; /* c2: poll pipe_in_drain output */
apr_pollfd_t pfd_out_prod; /* c2: poll pipe_out_prod output */
int has_final_response; /* final HTTP response passed on out */
apr_status_t last_err; /* APR_SUCCES or last error encountered in filters */
- struct h2_response_parser *parser; /* optional parser to catch H1 responses */
- volatile int done; /* c2: processing has finished */
+ apr_uint32_t started; /* c2: processing was started */
apr_time_t started_at; /* c2: when processing started */
+ apr_uint32_t done; /* c2: processing has finished */
Review comment:
good idea, will add the `/* volatile */` comments. *scratch*
What about adding `/* atomic */`? Seems even more clear.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@httpd.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@httpd.apache.org
For additional commands, e-mail: notifications-help@httpd.apache.org
[GitHub] [httpd] icing commented on pull request #297: H2 pollset impr
Posted by GitBox <gi...@apache.org>.
icing commented on pull request #297:
URL: https://github.com/apache/httpd/pull/297#issuecomment-1072249969
Merged as r1899032 in trunk. Thanks for reviewing!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@httpd.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@httpd.apache.org
For additional commands, e-mail: notifications-help@httpd.apache.org
[GitHub] [httpd] ylavic commented on a change in pull request #297: H2 pollset impr
Posted by GitBox <gi...@apache.org>.
ylavic commented on a change in pull request #297:
URL: https://github.com/apache/httpd/pull/297#discussion_r829038060
##########
File path: modules/http2/h2_mplx.c
##########
@@ -896,8 +940,8 @@ static void s_c2_done(h2_mplx *m, conn_rec *c2, h2_conn_ctx_t *conn_ctx)
ap_log_cerror(APLOG_MARK, APLOG_TRACE1, 0, c2,
"h2_mplx(%s-%d): c2 done", conn_ctx->id, conn_ctx->stream_id);
- ap_assert(conn_ctx->done == 0);
- conn_ctx->done = 1;
+ AP_DEBUG_ASSERT(apr_atomic_read32(&conn_ctx->done) == 0);
+ apr_atomic_set32(&conn_ctx->done, 1);
Review comment:
Oh this is the only place where `->done` is set (so not a TOCTOU issue, no CAS needed)..
Possibly `return;` on already set is meaningful still, or it should be an `ap_assert()`?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@httpd.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@httpd.apache.org
For additional commands, e-mail: notifications-help@httpd.apache.org
[GitHub] [httpd] icing closed pull request #297: H2 pollset impr
Posted by GitBox <gi...@apache.org>.
icing closed pull request #297:
URL: https://github.com/apache/httpd/pull/297
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@httpd.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@httpd.apache.org
For additional commands, e-mail: notifications-help@httpd.apache.org
[GitHub] [httpd] ylavic commented on a change in pull request #297: H2 pollset impr
Posted by GitBox <gi...@apache.org>.
ylavic commented on a change in pull request #297:
URL: https://github.com/apache/httpd/pull/297#discussion_r829031991
##########
File path: modules/http2/h2_conn_ctx.h
##########
@@ -48,20 +50,17 @@ struct h2_conn_ctx_t {
struct h2_bucket_beam *beam_out; /* c2: data out, created from req_pool */
struct h2_bucket_beam *beam_in; /* c2: data in or NULL, borrowed from request stream */
- apr_pool_t *mplx_pool; /* c2: an mplx child pool for safe use inside mplx lock */
apr_file_t *pipe_in_prod[2]; /* c2: input produced notification pipe */
- apr_file_t *pipe_in_drain[2]; /* c2: input drained notification pipe */
apr_file_t *pipe_out_prod[2]; /* c2: output produced notification pipe */
- apr_pollfd_t pfd_in_drain; /* c2: poll pipe_in_drain output */
apr_pollfd_t pfd_out_prod; /* c2: poll pipe_out_prod output */
int has_final_response; /* final HTTP response passed on out */
apr_status_t last_err; /* APR_SUCCES or last error encountered in filters */
- struct h2_response_parser *parser; /* optional parser to catch H1 responses */
- volatile int done; /* c2: processing has finished */
+ apr_uint32_t started; /* c2: processing was started */
apr_time_t started_at; /* c2: when processing started */
+ apr_uint32_t done; /* c2: processing has finished */
Review comment:
Maybe leave the `volatile` indicator for `started` and `done` to document that they are accessed atomically (now explicitely) still?
##########
File path: modules/http2/h2_mplx.c
##########
@@ -173,6 +176,63 @@ static void m_stream_cleanup(h2_mplx *m, h2_stream *stream)
}
}
+static h2_c2_transit *c2_transit_create(h2_mplx *m)
+{
+ apr_allocator_t *allocator;
+ apr_pool_t *ptrans;
+ h2_c2_transit *transit;
+ apr_status_t rv;
+
+ /* We create a pool with its own allocator to be used for
+ * processing a request. This is the only way to have the processing
+ * independent of its parent pool in the sense that it can work in
+ * another thread.
+ */
+
+ apr_allocator_create(&allocator);
Review comment:
This could fail on OOM, `return NULL` or simply `ap_abort_on_oom()` in this case?
Since allocation failures on ptrans will abort() I wonder if allocating ptrans itself shouldn't abort() too, thus here and in case of `apr_pool_create_ex()` below.
This'd make checking `c2_transit_{create,get}()` for NULL is the caller(s) moot.
##########
File path: modules/http2/h2_mplx.c
##########
@@ -254,18 +314,20 @@ h2_mplx *h2_mplx_c1_create(h2_stream *stream0, server_rec *s, apr_pool_t *parent
m->streams_ev_in = apr_array_make(m->pool, 10, sizeof(h2_stream*));
m->streams_ev_out = apr_array_make(m->pool, 10, sizeof(h2_stream*));
-#if !H2_POLL_STREAMS
+ m->streams_input_read = h2_iq_create(m->pool, 10);
status = apr_thread_mutex_create(&m->poll_lock, APR_THREAD_MUTEX_DEFAULT,
m->pool);
if (APR_SUCCESS != status) goto failure;
- m->streams_input_read = h2_iq_create(m->pool, 10);
+#if !H2_POLL_STREAMS
Review comment:
Can't we do the same thing for output streaming, i.e. wakeup pollset instead of pipe putc in `c2_beam_output_write_notify()`?
And then get rid of `H2_POLL_STREAMS`, but I may be missing something..
##########
File path: modules/http2/h2_mplx.c
##########
@@ -254,18 +314,20 @@ h2_mplx *h2_mplx_c1_create(h2_stream *stream0, server_rec *s, apr_pool_t *parent
m->streams_ev_in = apr_array_make(m->pool, 10, sizeof(h2_stream*));
m->streams_ev_out = apr_array_make(m->pool, 10, sizeof(h2_stream*));
-#if !H2_POLL_STREAMS
+ m->streams_input_read = h2_iq_create(m->pool, 10);
status = apr_thread_mutex_create(&m->poll_lock, APR_THREAD_MUTEX_DEFAULT,
m->pool);
if (APR_SUCCESS != status) goto failure;
- m->streams_input_read = h2_iq_create(m->pool, 10);
+#if !H2_POLL_STREAMS
m->streams_output_written = h2_iq_create(m->pool, 10);
#endif
conn_ctx = h2_conn_ctx_get(m->c1);
mplx_pollset_add(m, conn_ctx);
m->scratch_r = apr_pcalloc(m->pool, sizeof(*m->scratch_r));
+ m->max_spare_transits = 3;
Review comment:
Magic value `3` => `#define` or setting?
Also `m->max_spare_transits` is the same as `m->c2_transits->nalloc` so you might want to use the latter only all over the code?
##########
File path: modules/http2/h2_mplx.c
##########
@@ -896,8 +940,8 @@ static void s_c2_done(h2_mplx *m, conn_rec *c2, h2_conn_ctx_t *conn_ctx)
ap_log_cerror(APLOG_MARK, APLOG_TRACE1, 0, c2,
"h2_mplx(%s-%d): c2 done", conn_ctx->id, conn_ctx->stream_id);
- ap_assert(conn_ctx->done == 0);
- conn_ctx->done = 1;
+ AP_DEBUG_ASSERT(apr_atomic_read32(&conn_ctx->done) == 0);
+ apr_atomic_set32(&conn_ctx->done, 1);
Review comment:
TOCTOU, should this be:
if (apr_atomic_cas32(&conn_ctx->done, 1, 0) != 0) {
APR_DEBUG_ASSERT(0);
}
?
And maybe `return;` if already set too? It looks like `s_c2_done()` is not supposed to be called multiple times and if it happens httpd's teeth will fall out (or can we simply ignore it)?
##########
File path: modules/http2/h2_workers.c
##########
@@ -40,7 +40,7 @@ struct h2_slot {
apr_thread_t *thread;
apr_thread_mutex_t *lock;
apr_thread_cond_t *not_idle;
- volatile apr_uint32_t timed_out;
+ apr_uint32_t timed_out;
Review comment:
`volative` (or `/* volatile */`) might be documentary still
##########
File path: modules/http2/h2_workers.c
##########
@@ -127,10 +127,9 @@ static void wake_idle_worker(h2_workers *workers)
{
h2_slot *slot = pop_slot(&workers->idle);
if (slot) {
- int timed_out = 0;
+ int timed_out;
Review comment:
It looks like `wake_idle_worker()` could use a loop like:
```
for (;;) {
slot = pop_slot());
if (!slot) {
if (workers->dynamic && ...) {
...
}
break;
}
lock();
timed_out = ...;
if (!timed_out) {
signal();
unlock();
break;
}
unlock();
slot_done();
}
```
rather than a recursion?
##########
File path: modules/http2/h2_workers.h
##########
@@ -38,19 +38,17 @@ struct h2_workers {
int next_worker_id;
apr_uint32_t max_workers;
- volatile apr_uint32_t min_workers; /* is changed during graceful shutdown */
- volatile apr_interval_time_t max_idle_duration; /* is changed during graceful shutdown */
-
- volatile int aborted;
- volatile int shutdown;
+ apr_uint32_t min_workers;
+ apr_uint32_t worker_count;
+ apr_uint32_t max_idle_secs;
+ apr_uint32_t aborted;
+ apr_uint32_t shutdown;
Review comment:
Ditto for volatile "documentation" (for those which are still accessed atomically)
##########
File path: server/connection.c
##########
@@ -66,6 +70,13 @@ AP_DECLARE(conn_rec *) ap_create_connection(apr_pool_t *p,
return c;
}
+AP_DECLARE(conn_rec *) ap_create_secondary_connection(apr_pool_t *p,
+ conn_rec *master,
+ apr_bucket_alloc_t *alloc)
+{
+ return ap_run_create_secondary_connection(p, master, alloc);
+}
Review comment:
I'm not sure we need a new `ap_create_secondary_connection()` for now if it's only a pass through `ap_run_create_secondary_connection()`
##########
File path: server/core.c
##########
@@ -5517,6 +5519,55 @@ static conn_rec *core_create_conn(apr_pool_t *ptrans, server_rec *s,
return c;
}
+static conn_rec *core_create_secondary_conn(apr_pool_t *ptrans,
+ conn_rec *master,
+ apr_bucket_alloc_t *alloc)
+{
+ apr_pool_t *pool;
+ conn_config_t *conn_config;
+ conn_rec *c = (conn_rec *) apr_pcalloc(ptrans, sizeof(conn_rec));
+
+ /* Got a connection structure, so initialize what fields we can
+ * (the rest are zeroed out by pcalloc).
+ */
+ apr_pool_create(&pool, ptrans);
+ apr_pool_tag(pool, "secondary_conn");
+
+ /* first copy everything, then replace what is needed */
+ memcpy(c, master, sizeof(*c));
Review comment:
pcalloc + memcpy => pmemdup ?
##########
File path: modules/http2/h2_mplx.c
##########
@@ -896,8 +940,8 @@ static void s_c2_done(h2_mplx *m, conn_rec *c2, h2_conn_ctx_t *conn_ctx)
ap_log_cerror(APLOG_MARK, APLOG_TRACE1, 0, c2,
"h2_mplx(%s-%d): c2 done", conn_ctx->id, conn_ctx->stream_id);
- ap_assert(conn_ctx->done == 0);
- conn_ctx->done = 1;
+ AP_DEBUG_ASSERT(apr_atomic_read32(&conn_ctx->done) == 0);
+ apr_atomic_set32(&conn_ctx->done, 1);
Review comment:
TOCTOU, should this be:
```
if (apr_atomic_cas32(&conn_ctx->done, 1, 0) != 0) {
APR_DEBUG_ASSERT(0);
}
```
?
And maybe `return;` if already set too? It looks like `s_c2_done()` is not supposed to be called multiple times and if it happens httpd's teeth will fall out (or can we simply ignore it)?
##########
File path: modules/http2/h2_mplx.c
##########
@@ -896,8 +940,8 @@ static void s_c2_done(h2_mplx *m, conn_rec *c2, h2_conn_ctx_t *conn_ctx)
ap_log_cerror(APLOG_MARK, APLOG_TRACE1, 0, c2,
"h2_mplx(%s-%d): c2 done", conn_ctx->id, conn_ctx->stream_id);
- ap_assert(conn_ctx->done == 0);
- conn_ctx->done = 1;
+ AP_DEBUG_ASSERT(apr_atomic_read32(&conn_ctx->done) == 0);
+ apr_atomic_set32(&conn_ctx->done, 1);
Review comment:
Oh this is the only place where `->done` (so not a TOCTOU issue, no CAS needed)..
Probaly `return;` on already set is meaningful still, or it should be an `ap_assert()`?
##########
File path: modules/http2/h2_mplx.c
##########
@@ -896,8 +940,8 @@ static void s_c2_done(h2_mplx *m, conn_rec *c2, h2_conn_ctx_t *conn_ctx)
ap_log_cerror(APLOG_MARK, APLOG_TRACE1, 0, c2,
"h2_mplx(%s-%d): c2 done", conn_ctx->id, conn_ctx->stream_id);
- ap_assert(conn_ctx->done == 0);
- conn_ctx->done = 1;
+ AP_DEBUG_ASSERT(apr_atomic_read32(&conn_ctx->done) == 0);
+ apr_atomic_set32(&conn_ctx->done, 1);
Review comment:
Oh this is the only place where `->done` is set (so not a TOCTOU issue, no CAS needed)..
Possibly `return;` on already set is meaningful still, or it should be an `ap_assert()`?
##########
File path: modules/http2/h2_workers.c
##########
@@ -127,10 +127,9 @@ static void wake_idle_worker(h2_workers *workers)
{
h2_slot *slot = pop_slot(&workers->idle);
if (slot) {
- int timed_out = 0;
+ int timed_out;
Review comment:
It looks like `wake_idle_worker()` could use a loop like:
```
for (;;) {
slot = pop_slot());
if (!slot) {
if (workers->dynamic && ...) {
add_worker(workers);
}
return;
}
if (!apr_atomic_read32(&slot->timed_out)) {
lock();
signal();
unlock();
return;
}
slot_done();
}
```
rather than a recursion?
##########
File path: server/connection.c
##########
@@ -66,6 +70,13 @@ AP_DECLARE(conn_rec *) ap_create_connection(apr_pool_t *p,
return c;
}
+AP_DECLARE(conn_rec *) ap_create_secondary_connection(apr_pool_t *p,
+ conn_rec *master,
+ apr_bucket_alloc_t *alloc)
+{
+ return ap_run_create_secondary_connection(p, master, alloc);
+}
Review comment:
Yeah right but if at that time for instance we need more parameters in the helper than in the hook we'd have to create and _ex() version if the helper exists already.
Not a strong opinion though..
##########
File path: modules/http2/h2_mplx.c
##########
@@ -254,18 +314,20 @@ h2_mplx *h2_mplx_c1_create(h2_stream *stream0, server_rec *s, apr_pool_t *parent
m->streams_ev_in = apr_array_make(m->pool, 10, sizeof(h2_stream*));
m->streams_ev_out = apr_array_make(m->pool, 10, sizeof(h2_stream*));
-#if !H2_POLL_STREAMS
+ m->streams_input_read = h2_iq_create(m->pool, 10);
status = apr_thread_mutex_create(&m->poll_lock, APR_THREAD_MUTEX_DEFAULT,
m->pool);
if (APR_SUCCESS != status) goto failure;
- m->streams_input_read = h2_iq_create(m->pool, 10);
+#if !H2_POLL_STREAMS
Review comment:
Oh, I wasn't thinking of removing file descriptors but wondered how you managed to remove the `H2_POLL_STREAMS` support for `streams_input_read` and why you couldn't do the same for the output side.
##########
File path: modules/http2/h2_conn_ctx.h
##########
@@ -48,20 +50,17 @@ struct h2_conn_ctx_t {
struct h2_bucket_beam *beam_out; /* c2: data out, created from req_pool */
struct h2_bucket_beam *beam_in; /* c2: data in or NULL, borrowed from request stream */
- apr_pool_t *mplx_pool; /* c2: an mplx child pool for safe use inside mplx lock */
apr_file_t *pipe_in_prod[2]; /* c2: input produced notification pipe */
- apr_file_t *pipe_in_drain[2]; /* c2: input drained notification pipe */
apr_file_t *pipe_out_prod[2]; /* c2: output produced notification pipe */
- apr_pollfd_t pfd_in_drain; /* c2: poll pipe_in_drain output */
apr_pollfd_t pfd_out_prod; /* c2: poll pipe_out_prod output */
int has_final_response; /* final HTTP response passed on out */
apr_status_t last_err; /* APR_SUCCES or last error encountered in filters */
- struct h2_response_parser *parser; /* optional parser to catch H1 responses */
- volatile int done; /* c2: processing has finished */
+ apr_uint32_t started; /* c2: processing was started */
apr_time_t started_at; /* c2: when processing started */
+ apr_uint32_t done; /* c2: processing has finished */
Review comment:
+1
##########
File path: modules/http2/h2_mplx.c
##########
@@ -896,8 +940,8 @@ static void s_c2_done(h2_mplx *m, conn_rec *c2, h2_conn_ctx_t *conn_ctx)
ap_log_cerror(APLOG_MARK, APLOG_TRACE1, 0, c2,
"h2_mplx(%s-%d): c2 done", conn_ctx->id, conn_ctx->stream_id);
- ap_assert(conn_ctx->done == 0);
- conn_ctx->done = 1;
+ AP_DEBUG_ASSERT(apr_atomic_read32(&conn_ctx->done) == 0);
+ apr_atomic_set32(&conn_ctx->done, 1);
Review comment:
Fair enough.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@httpd.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@httpd.apache.org
For additional commands, e-mail: notifications-help@httpd.apache.org
[GitHub] [httpd] icing commented on a change in pull request #297: H2 pollset impr
Posted by GitBox <gi...@apache.org>.
icing commented on a change in pull request #297:
URL: https://github.com/apache/httpd/pull/297#discussion_r829080701
##########
File path: server/core.c
##########
@@ -5517,6 +5519,55 @@ static conn_rec *core_create_conn(apr_pool_t *ptrans, server_rec *s,
return c;
}
+static conn_rec *core_create_secondary_conn(apr_pool_t *ptrans,
+ conn_rec *master,
+ apr_bucket_alloc_t *alloc)
+{
+ apr_pool_t *pool;
+ conn_config_t *conn_config;
+ conn_rec *c = (conn_rec *) apr_pcalloc(ptrans, sizeof(conn_rec));
+
+ /* Got a connection structure, so initialize what fields we can
+ * (the rest are zeroed out by pcalloc).
+ */
+ apr_pool_create(&pool, ptrans);
+ apr_pool_tag(pool, "secondary_conn");
+
+ /* first copy everything, then replace what is needed */
+ memcpy(c, master, sizeof(*c));
Review comment:
Changed as suggested.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@httpd.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@httpd.apache.org
For additional commands, e-mail: notifications-help@httpd.apache.org
[GitHub] [httpd] ylavic commented on a change in pull request #297: H2 pollset impr
Posted by GitBox <gi...@apache.org>.
ylavic commented on a change in pull request #297:
URL: https://github.com/apache/httpd/pull/297#discussion_r829031991
##########
File path: modules/http2/h2_conn_ctx.h
##########
@@ -48,20 +50,17 @@ struct h2_conn_ctx_t {
struct h2_bucket_beam *beam_out; /* c2: data out, created from req_pool */
struct h2_bucket_beam *beam_in; /* c2: data in or NULL, borrowed from request stream */
- apr_pool_t *mplx_pool; /* c2: an mplx child pool for safe use inside mplx lock */
apr_file_t *pipe_in_prod[2]; /* c2: input produced notification pipe */
- apr_file_t *pipe_in_drain[2]; /* c2: input drained notification pipe */
apr_file_t *pipe_out_prod[2]; /* c2: output produced notification pipe */
- apr_pollfd_t pfd_in_drain; /* c2: poll pipe_in_drain output */
apr_pollfd_t pfd_out_prod; /* c2: poll pipe_out_prod output */
int has_final_response; /* final HTTP response passed on out */
apr_status_t last_err; /* APR_SUCCES or last error encountered in filters */
- struct h2_response_parser *parser; /* optional parser to catch H1 responses */
- volatile int done; /* c2: processing has finished */
+ apr_uint32_t started; /* c2: processing was started */
apr_time_t started_at; /* c2: when processing started */
+ apr_uint32_t done; /* c2: processing has finished */
Review comment:
Maybe leave the `volatile` indicator for `started` and `done` to document that they are accessed atomically (now explicitely) still?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@httpd.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@httpd.apache.org
For additional commands, e-mail: notifications-help@httpd.apache.org
[GitHub] [httpd] icing commented on a change in pull request #297: H2 pollset impr
Posted by GitBox <gi...@apache.org>.
icing commented on a change in pull request #297:
URL: https://github.com/apache/httpd/pull/297#discussion_r829079966
##########
File path: modules/http2/h2_conn_ctx.h
##########
@@ -48,20 +50,17 @@ struct h2_conn_ctx_t {
struct h2_bucket_beam *beam_out; /* c2: data out, created from req_pool */
struct h2_bucket_beam *beam_in; /* c2: data in or NULL, borrowed from request stream */
- apr_pool_t *mplx_pool; /* c2: an mplx child pool for safe use inside mplx lock */
apr_file_t *pipe_in_prod[2]; /* c2: input produced notification pipe */
- apr_file_t *pipe_in_drain[2]; /* c2: input drained notification pipe */
apr_file_t *pipe_out_prod[2]; /* c2: output produced notification pipe */
- apr_pollfd_t pfd_in_drain; /* c2: poll pipe_in_drain output */
apr_pollfd_t pfd_out_prod; /* c2: poll pipe_out_prod output */
int has_final_response; /* final HTTP response passed on out */
apr_status_t last_err; /* APR_SUCCES or last error encountered in filters */
- struct h2_response_parser *parser; /* optional parser to catch H1 responses */
- volatile int done; /* c2: processing has finished */
+ apr_uint32_t started; /* c2: processing was started */
apr_time_t started_at; /* c2: when processing started */
+ apr_uint32_t done; /* c2: processing has finished */
Review comment:
added
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@httpd.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@httpd.apache.org
For additional commands, e-mail: notifications-help@httpd.apache.org
[GitHub] [httpd] icing commented on a change in pull request #297: H2 pollset impr
Posted by GitBox <gi...@apache.org>.
icing commented on a change in pull request #297:
URL: https://github.com/apache/httpd/pull/297#discussion_r829080350
##########
File path: modules/http2/h2_workers.c
##########
@@ -40,7 +40,7 @@ struct h2_slot {
apr_thread_t *thread;
apr_thread_mutex_t *lock;
apr_thread_cond_t *not_idle;
- volatile apr_uint32_t timed_out;
+ apr_uint32_t timed_out;
Review comment:
added
##########
File path: modules/http2/h2_workers.c
##########
@@ -127,10 +127,9 @@ static void wake_idle_worker(h2_workers *workers)
{
h2_slot *slot = pop_slot(&workers->idle);
if (slot) {
- int timed_out = 0;
+ int timed_out;
Review comment:
Changed as suggested.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@httpd.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@httpd.apache.org
For additional commands, e-mail: notifications-help@httpd.apache.org
[GitHub] [httpd] icing closed pull request #297: H2 pollset impr
Posted by GitBox <gi...@apache.org>.
icing closed pull request #297:
URL: https://github.com/apache/httpd/pull/297
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@httpd.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@httpd.apache.org
For additional commands, e-mail: notifications-help@httpd.apache.org
[GitHub] [httpd] icing commented on a change in pull request #297: H2 pollset impr
Posted by GitBox <gi...@apache.org>.
icing commented on a change in pull request #297:
URL: https://github.com/apache/httpd/pull/297#discussion_r829051246
##########
File path: modules/http2/h2_conn_ctx.h
##########
@@ -48,20 +50,17 @@ struct h2_conn_ctx_t {
struct h2_bucket_beam *beam_out; /* c2: data out, created from req_pool */
struct h2_bucket_beam *beam_in; /* c2: data in or NULL, borrowed from request stream */
- apr_pool_t *mplx_pool; /* c2: an mplx child pool for safe use inside mplx lock */
apr_file_t *pipe_in_prod[2]; /* c2: input produced notification pipe */
- apr_file_t *pipe_in_drain[2]; /* c2: input drained notification pipe */
apr_file_t *pipe_out_prod[2]; /* c2: output produced notification pipe */
- apr_pollfd_t pfd_in_drain; /* c2: poll pipe_in_drain output */
apr_pollfd_t pfd_out_prod; /* c2: poll pipe_out_prod output */
int has_final_response; /* final HTTP response passed on out */
apr_status_t last_err; /* APR_SUCCES or last error encountered in filters */
- struct h2_response_parser *parser; /* optional parser to catch H1 responses */
- volatile int done; /* c2: processing has finished */
+ apr_uint32_t started; /* c2: processing was started */
apr_time_t started_at; /* c2: when processing started */
+ apr_uint32_t done; /* c2: processing has finished */
Review comment:
good idea, will add the `/* volatile */` comments.
##########
File path: modules/http2/h2_mplx.c
##########
@@ -254,18 +314,20 @@ h2_mplx *h2_mplx_c1_create(h2_stream *stream0, server_rec *s, apr_pool_t *parent
m->streams_ev_in = apr_array_make(m->pool, 10, sizeof(h2_stream*));
m->streams_ev_out = apr_array_make(m->pool, 10, sizeof(h2_stream*));
-#if !H2_POLL_STREAMS
+ m->streams_input_read = h2_iq_create(m->pool, 10);
status = apr_thread_mutex_create(&m->poll_lock, APR_THREAD_MUTEX_DEFAULT,
m->pool);
if (APR_SUCCESS != status) goto failure;
- m->streams_input_read = h2_iq_create(m->pool, 10);
+#if !H2_POLL_STREAMS
Review comment:
I thought about this. We have support for disabling this bc Windows and older APR versions.
The possible benefit of the file descriptors is that we might adapt `mod_proxy` modules to use them for shuffling content back and forth, as you did for the wstunnel.
But then again, as long as we do not have that on all platforms we support, this means different code paths and complexity.
##########
File path: modules/http2/h2_mplx.c
##########
@@ -254,18 +314,20 @@ h2_mplx *h2_mplx_c1_create(h2_stream *stream0, server_rec *s, apr_pool_t *parent
m->streams_ev_in = apr_array_make(m->pool, 10, sizeof(h2_stream*));
m->streams_ev_out = apr_array_make(m->pool, 10, sizeof(h2_stream*));
-#if !H2_POLL_STREAMS
+ m->streams_input_read = h2_iq_create(m->pool, 10);
status = apr_thread_mutex_create(&m->poll_lock, APR_THREAD_MUTEX_DEFAULT,
m->pool);
if (APR_SUCCESS != status) goto failure;
- m->streams_input_read = h2_iq_create(m->pool, 10);
+#if !H2_POLL_STREAMS
m->streams_output_written = h2_iq_create(m->pool, 10);
#endif
conn_ctx = h2_conn_ctx_get(m->c1);
mplx_pollset_add(m, conn_ctx);
m->scratch_r = apr_pcalloc(m->pool, sizeof(*m->scratch_r));
+ m->max_spare_transits = 3;
Review comment:
This was a start setting and I plan to continue work on this.
Atm I am considering a global reserve list, because when connection go idle, this just hangs around useless.
The global list should probably keep not more than H2MaxWorker transits around.
##########
File path: modules/http2/h2_mplx.c
##########
@@ -896,8 +940,8 @@ static void s_c2_done(h2_mplx *m, conn_rec *c2, h2_conn_ctx_t *conn_ctx)
ap_log_cerror(APLOG_MARK, APLOG_TRACE1, 0, c2,
"h2_mplx(%s-%d): c2 done", conn_ctx->id, conn_ctx->stream_id);
- ap_assert(conn_ctx->done == 0);
- conn_ctx->done = 1;
+ AP_DEBUG_ASSERT(apr_atomic_read32(&conn_ctx->done) == 0);
+ apr_atomic_set32(&conn_ctx->done, 1);
Review comment:
A DEBUG_ASSERT() seems best here. It is really only a dev check if things got screwed up by a change. This method needs to be called once and only once for a c2 connection.
##########
File path: modules/http2/h2_workers.c
##########
@@ -40,7 +40,7 @@ struct h2_slot {
apr_thread_t *thread;
apr_thread_mutex_t *lock;
apr_thread_cond_t *not_idle;
- volatile apr_uint32_t timed_out;
+ apr_uint32_t timed_out;
Review comment:
ack
##########
File path: modules/http2/h2_workers.h
##########
@@ -38,19 +38,17 @@ struct h2_workers {
int next_worker_id;
apr_uint32_t max_workers;
- volatile apr_uint32_t min_workers; /* is changed during graceful shutdown */
- volatile apr_interval_time_t max_idle_duration; /* is changed during graceful shutdown */
-
- volatile int aborted;
- volatile int shutdown;
+ apr_uint32_t min_workers;
+ apr_uint32_t worker_count;
+ apr_uint32_t max_idle_secs;
+ apr_uint32_t aborted;
+ apr_uint32_t shutdown;
Review comment:
ack.
##########
File path: server/connection.c
##########
@@ -66,6 +70,13 @@ AP_DECLARE(conn_rec *) ap_create_connection(apr_pool_t *p,
return c;
}
+AP_DECLARE(conn_rec *) ap_create_secondary_connection(apr_pool_t *p,
+ conn_rec *master,
+ apr_bucket_alloc_t *alloc)
+{
+ return ap_run_create_secondary_connection(p, master, alloc);
+}
Review comment:
I think it is good to have and at no cost. We have added wrappers around hooks in the past, when things turned out to need global checks or additional things.
##########
File path: server/core.c
##########
@@ -5517,6 +5519,55 @@ static conn_rec *core_create_conn(apr_pool_t *ptrans, server_rec *s,
return c;
}
+static conn_rec *core_create_secondary_conn(apr_pool_t *ptrans,
+ conn_rec *master,
+ apr_bucket_alloc_t *alloc)
+{
+ apr_pool_t *pool;
+ conn_config_t *conn_config;
+ conn_rec *c = (conn_rec *) apr_pcalloc(ptrans, sizeof(conn_rec));
+
+ /* Got a connection structure, so initialize what fields we can
+ * (the rest are zeroed out by pcalloc).
+ */
+ apr_pool_create(&pool, ptrans);
+ apr_pool_tag(pool, "secondary_conn");
+
+ /* first copy everything, then replace what is needed */
+ memcpy(c, master, sizeof(*c));
Review comment:
good point.
##########
File path: modules/http2/h2_conn_ctx.h
##########
@@ -48,20 +50,17 @@ struct h2_conn_ctx_t {
struct h2_bucket_beam *beam_out; /* c2: data out, created from req_pool */
struct h2_bucket_beam *beam_in; /* c2: data in or NULL, borrowed from request stream */
- apr_pool_t *mplx_pool; /* c2: an mplx child pool for safe use inside mplx lock */
apr_file_t *pipe_in_prod[2]; /* c2: input produced notification pipe */
- apr_file_t *pipe_in_drain[2]; /* c2: input drained notification pipe */
apr_file_t *pipe_out_prod[2]; /* c2: output produced notification pipe */
- apr_pollfd_t pfd_in_drain; /* c2: poll pipe_in_drain output */
apr_pollfd_t pfd_out_prod; /* c2: poll pipe_out_prod output */
int has_final_response; /* final HTTP response passed on out */
apr_status_t last_err; /* APR_SUCCES or last error encountered in filters */
- struct h2_response_parser *parser; /* optional parser to catch H1 responses */
- volatile int done; /* c2: processing has finished */
+ apr_uint32_t started; /* c2: processing was started */
apr_time_t started_at; /* c2: when processing started */
+ apr_uint32_t done; /* c2: processing has finished */
Review comment:
good idea, will add the `/* volatile */` comments. *scratch*
What about adding `/* atomic */`? Seems even more clear.
##########
File path: modules/http2/h2_conn_ctx.h
##########
@@ -48,20 +50,17 @@ struct h2_conn_ctx_t {
struct h2_bucket_beam *beam_out; /* c2: data out, created from req_pool */
struct h2_bucket_beam *beam_in; /* c2: data in or NULL, borrowed from request stream */
- apr_pool_t *mplx_pool; /* c2: an mplx child pool for safe use inside mplx lock */
apr_file_t *pipe_in_prod[2]; /* c2: input produced notification pipe */
- apr_file_t *pipe_in_drain[2]; /* c2: input drained notification pipe */
apr_file_t *pipe_out_prod[2]; /* c2: output produced notification pipe */
- apr_pollfd_t pfd_in_drain; /* c2: poll pipe_in_drain output */
apr_pollfd_t pfd_out_prod; /* c2: poll pipe_out_prod output */
int has_final_response; /* final HTTP response passed on out */
apr_status_t last_err; /* APR_SUCCES or last error encountered in filters */
- struct h2_response_parser *parser; /* optional parser to catch H1 responses */
- volatile int done; /* c2: processing has finished */
+ apr_uint32_t started; /* c2: processing was started */
apr_time_t started_at; /* c2: when processing started */
+ apr_uint32_t done; /* c2: processing has finished */
Review comment:
added
##########
File path: modules/http2/h2_workers.c
##########
@@ -40,7 +40,7 @@ struct h2_slot {
apr_thread_t *thread;
apr_thread_mutex_t *lock;
apr_thread_cond_t *not_idle;
- volatile apr_uint32_t timed_out;
+ apr_uint32_t timed_out;
Review comment:
added
##########
File path: modules/http2/h2_workers.c
##########
@@ -127,10 +127,9 @@ static void wake_idle_worker(h2_workers *workers)
{
h2_slot *slot = pop_slot(&workers->idle);
if (slot) {
- int timed_out = 0;
+ int timed_out;
Review comment:
Changed as suggested.
##########
File path: server/core.c
##########
@@ -5517,6 +5519,55 @@ static conn_rec *core_create_conn(apr_pool_t *ptrans, server_rec *s,
return c;
}
+static conn_rec *core_create_secondary_conn(apr_pool_t *ptrans,
+ conn_rec *master,
+ apr_bucket_alloc_t *alloc)
+{
+ apr_pool_t *pool;
+ conn_config_t *conn_config;
+ conn_rec *c = (conn_rec *) apr_pcalloc(ptrans, sizeof(conn_rec));
+
+ /* Got a connection structure, so initialize what fields we can
+ * (the rest are zeroed out by pcalloc).
+ */
+ apr_pool_create(&pool, ptrans);
+ apr_pool_tag(pool, "secondary_conn");
+
+ /* first copy everything, then replace what is needed */
+ memcpy(c, master, sizeof(*c));
Review comment:
Changed as suggested.
##########
File path: server/connection.c
##########
@@ -66,6 +70,13 @@ AP_DECLARE(conn_rec *) ap_create_connection(apr_pool_t *p,
return c;
}
+AP_DECLARE(conn_rec *) ap_create_secondary_connection(apr_pool_t *p,
+ conn_rec *master,
+ apr_bucket_alloc_t *alloc)
+{
+ return ap_run_create_secondary_connection(p, master, alloc);
+}
Review comment:
Not opposed to it either. Maybe @rpluem can arbitrate...
##########
File path: modules/http2/h2_mplx.c
##########
@@ -173,6 +176,63 @@ static void m_stream_cleanup(h2_mplx *m, h2_stream *stream)
}
}
+static h2_c2_transit *c2_transit_create(h2_mplx *m)
+{
+ apr_allocator_t *allocator;
+ apr_pool_t *ptrans;
+ h2_c2_transit *transit;
+ apr_status_t rv;
+
+ /* We create a pool with its own allocator to be used for
+ * processing a request. This is the only way to have the processing
+ * independent of its parent pool in the sense that it can work in
+ * another thread.
+ */
+
+ apr_allocator_create(&allocator);
Review comment:
Added the checks. Thanks.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@httpd.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@httpd.apache.org
For additional commands, e-mail: notifications-help@httpd.apache.org
[GitHub] [httpd] icing commented on a change in pull request #297: H2 pollset impr
Posted by GitBox <gi...@apache.org>.
icing commented on a change in pull request #297:
URL: https://github.com/apache/httpd/pull/297#discussion_r829057013
##########
File path: modules/http2/h2_workers.c
##########
@@ -40,7 +40,7 @@ struct h2_slot {
apr_thread_t *thread;
apr_thread_mutex_t *lock;
apr_thread_cond_t *not_idle;
- volatile apr_uint32_t timed_out;
+ apr_uint32_t timed_out;
Review comment:
ack
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@httpd.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@httpd.apache.org
For additional commands, e-mail: notifications-help@httpd.apache.org
[GitHub] [httpd] ylavic commented on a change in pull request #297: H2 pollset impr
Posted by GitBox <gi...@apache.org>.
ylavic commented on a change in pull request #297:
URL: https://github.com/apache/httpd/pull/297#discussion_r829094701
##########
File path: modules/http2/h2_mplx.c
##########
@@ -896,8 +940,8 @@ static void s_c2_done(h2_mplx *m, conn_rec *c2, h2_conn_ctx_t *conn_ctx)
ap_log_cerror(APLOG_MARK, APLOG_TRACE1, 0, c2,
"h2_mplx(%s-%d): c2 done", conn_ctx->id, conn_ctx->stream_id);
- ap_assert(conn_ctx->done == 0);
- conn_ctx->done = 1;
+ AP_DEBUG_ASSERT(apr_atomic_read32(&conn_ctx->done) == 0);
+ apr_atomic_set32(&conn_ctx->done, 1);
Review comment:
Fair enough.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@httpd.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@httpd.apache.org
For additional commands, e-mail: notifications-help@httpd.apache.org
[GitHub] [httpd] ylavic commented on a change in pull request #297: H2 pollset impr
Posted by GitBox <gi...@apache.org>.
ylavic commented on a change in pull request #297:
URL: https://github.com/apache/httpd/pull/297#discussion_r829038060
##########
File path: modules/http2/h2_mplx.c
##########
@@ -896,8 +940,8 @@ static void s_c2_done(h2_mplx *m, conn_rec *c2, h2_conn_ctx_t *conn_ctx)
ap_log_cerror(APLOG_MARK, APLOG_TRACE1, 0, c2,
"h2_mplx(%s-%d): c2 done", conn_ctx->id, conn_ctx->stream_id);
- ap_assert(conn_ctx->done == 0);
- conn_ctx->done = 1;
+ AP_DEBUG_ASSERT(apr_atomic_read32(&conn_ctx->done) == 0);
+ apr_atomic_set32(&conn_ctx->done, 1);
Review comment:
Oh this is the only place where `->done` (so not a TOCTOU issue, no CAS needed)..
Probaly `return;` on already set is meaningful still, or it should be an `ap_assert()`?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@httpd.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@httpd.apache.org
For additional commands, e-mail: notifications-help@httpd.apache.org
[GitHub] [httpd] ylavic commented on a change in pull request #297: H2 pollset impr
Posted by GitBox <gi...@apache.org>.
ylavic commented on a change in pull request #297:
URL: https://github.com/apache/httpd/pull/297#discussion_r829032172
##########
File path: modules/http2/h2_mplx.c
##########
@@ -173,6 +176,63 @@ static void m_stream_cleanup(h2_mplx *m, h2_stream *stream)
}
}
+static h2_c2_transit *c2_transit_create(h2_mplx *m)
+{
+ apr_allocator_t *allocator;
+ apr_pool_t *ptrans;
+ h2_c2_transit *transit;
+ apr_status_t rv;
+
+ /* We create a pool with its own allocator to be used for
+ * processing a request. This is the only way to have the processing
+ * independent of its parent pool in the sense that it can work in
+ * another thread.
+ */
+
+ apr_allocator_create(&allocator);
Review comment:
This could fail on OOM, `return NULL` or simply `ap_abort_on_oom()` in this case?
Since allocation failures on ptrans will abort() I wonder if allocating ptrans itself shouldn't abort() too, thus here and in case of `apr_pool_create_ex()` below.
This'd make checking `c2_transit_{create,get}()` for NULL is the caller(s) moot.
##########
File path: modules/http2/h2_mplx.c
##########
@@ -254,18 +314,20 @@ h2_mplx *h2_mplx_c1_create(h2_stream *stream0, server_rec *s, apr_pool_t *parent
m->streams_ev_in = apr_array_make(m->pool, 10, sizeof(h2_stream*));
m->streams_ev_out = apr_array_make(m->pool, 10, sizeof(h2_stream*));
-#if !H2_POLL_STREAMS
+ m->streams_input_read = h2_iq_create(m->pool, 10);
status = apr_thread_mutex_create(&m->poll_lock, APR_THREAD_MUTEX_DEFAULT,
m->pool);
if (APR_SUCCESS != status) goto failure;
- m->streams_input_read = h2_iq_create(m->pool, 10);
+#if !H2_POLL_STREAMS
Review comment:
Can't we do the same thing for output streaming, i.e. wakeup pollset instead of pipe putc in `c2_beam_output_write_notify()`?
And then get rid of `H2_POLL_STREAMS`, but I may be missing something..
##########
File path: modules/http2/h2_mplx.c
##########
@@ -254,18 +314,20 @@ h2_mplx *h2_mplx_c1_create(h2_stream *stream0, server_rec *s, apr_pool_t *parent
m->streams_ev_in = apr_array_make(m->pool, 10, sizeof(h2_stream*));
m->streams_ev_out = apr_array_make(m->pool, 10, sizeof(h2_stream*));
-#if !H2_POLL_STREAMS
+ m->streams_input_read = h2_iq_create(m->pool, 10);
status = apr_thread_mutex_create(&m->poll_lock, APR_THREAD_MUTEX_DEFAULT,
m->pool);
if (APR_SUCCESS != status) goto failure;
- m->streams_input_read = h2_iq_create(m->pool, 10);
+#if !H2_POLL_STREAMS
m->streams_output_written = h2_iq_create(m->pool, 10);
#endif
conn_ctx = h2_conn_ctx_get(m->c1);
mplx_pollset_add(m, conn_ctx);
m->scratch_r = apr_pcalloc(m->pool, sizeof(*m->scratch_r));
+ m->max_spare_transits = 3;
Review comment:
Magic value `3` => `#define` or setting?
Also `m->max_spare_transits` is the same as `m->c2_transits->nalloc` so you might want to use the latter only all over the code?
##########
File path: modules/http2/h2_mplx.c
##########
@@ -896,8 +940,8 @@ static void s_c2_done(h2_mplx *m, conn_rec *c2, h2_conn_ctx_t *conn_ctx)
ap_log_cerror(APLOG_MARK, APLOG_TRACE1, 0, c2,
"h2_mplx(%s-%d): c2 done", conn_ctx->id, conn_ctx->stream_id);
- ap_assert(conn_ctx->done == 0);
- conn_ctx->done = 1;
+ AP_DEBUG_ASSERT(apr_atomic_read32(&conn_ctx->done) == 0);
+ apr_atomic_set32(&conn_ctx->done, 1);
Review comment:
TOCTOU, should this be:
if (apr_atomic_cas32(&conn_ctx->done, 1, 0) != 0) {
APR_DEBUG_ASSERT(0);
}
?
And maybe `return;` if already set too? It looks like `s_c2_done()` is not supposed to be called multiple times and if it happens httpd's teeth will fall out (or can we simply ignore it)?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@httpd.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@httpd.apache.org
For additional commands, e-mail: notifications-help@httpd.apache.org
[GitHub] [httpd] ylavic commented on a change in pull request #297: H2 pollset impr
Posted by GitBox <gi...@apache.org>.
ylavic commented on a change in pull request #297:
URL: https://github.com/apache/httpd/pull/297#discussion_r829033089
##########
File path: modules/http2/h2_workers.h
##########
@@ -38,19 +38,17 @@ struct h2_workers {
int next_worker_id;
apr_uint32_t max_workers;
- volatile apr_uint32_t min_workers; /* is changed during graceful shutdown */
- volatile apr_interval_time_t max_idle_duration; /* is changed during graceful shutdown */
-
- volatile int aborted;
- volatile int shutdown;
+ apr_uint32_t min_workers;
+ apr_uint32_t worker_count;
+ apr_uint32_t max_idle_secs;
+ apr_uint32_t aborted;
+ apr_uint32_t shutdown;
Review comment:
Ditto for volatile "documentation" (for those which are still accessed atomically)
##########
File path: server/connection.c
##########
@@ -66,6 +70,13 @@ AP_DECLARE(conn_rec *) ap_create_connection(apr_pool_t *p,
return c;
}
+AP_DECLARE(conn_rec *) ap_create_secondary_connection(apr_pool_t *p,
+ conn_rec *master,
+ apr_bucket_alloc_t *alloc)
+{
+ return ap_run_create_secondary_connection(p, master, alloc);
+}
Review comment:
I'm not sure we need a new `ap_create_secondary_connection()` for now if it's only a pass through `ap_run_create_secondary_connection()`
##########
File path: server/core.c
##########
@@ -5517,6 +5519,55 @@ static conn_rec *core_create_conn(apr_pool_t *ptrans, server_rec *s,
return c;
}
+static conn_rec *core_create_secondary_conn(apr_pool_t *ptrans,
+ conn_rec *master,
+ apr_bucket_alloc_t *alloc)
+{
+ apr_pool_t *pool;
+ conn_config_t *conn_config;
+ conn_rec *c = (conn_rec *) apr_pcalloc(ptrans, sizeof(conn_rec));
+
+ /* Got a connection structure, so initialize what fields we can
+ * (the rest are zeroed out by pcalloc).
+ */
+ apr_pool_create(&pool, ptrans);
+ apr_pool_tag(pool, "secondary_conn");
+
+ /* first copy everything, then replace what is needed */
+ memcpy(c, master, sizeof(*c));
Review comment:
pcalloc + memcpy => pmemdup ?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@httpd.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@httpd.apache.org
For additional commands, e-mail: notifications-help@httpd.apache.org
[GitHub] [httpd] ylavic commented on a change in pull request #297: H2 pollset impr
Posted by GitBox <gi...@apache.org>.
ylavic commented on a change in pull request #297:
URL: https://github.com/apache/httpd/pull/297#discussion_r829032958
##########
File path: modules/http2/h2_workers.c
##########
@@ -127,10 +127,9 @@ static void wake_idle_worker(h2_workers *workers)
{
h2_slot *slot = pop_slot(&workers->idle);
if (slot) {
- int timed_out = 0;
+ int timed_out;
Review comment:
It looks like `wake_idle_worker()` could use a loop like:
```
for (;;) {
slot = pop_slot());
if (!slot) {
if (workers->dynamic && ...) {
add_worker(workers);
}
return;
}
if (!apr_atomic_read32(&slot->timed_out)) {
lock();
signal();
unlock();
return;
}
slot_done();
}
```
rather than a recursion?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@httpd.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@httpd.apache.org
For additional commands, e-mail: notifications-help@httpd.apache.org
[GitHub] [httpd] icing commented on a change in pull request #297: H2 pollset impr
Posted by GitBox <gi...@apache.org>.
icing commented on a change in pull request #297:
URL: https://github.com/apache/httpd/pull/297#discussion_r829051246
##########
File path: modules/http2/h2_conn_ctx.h
##########
@@ -48,20 +50,17 @@ struct h2_conn_ctx_t {
struct h2_bucket_beam *beam_out; /* c2: data out, created from req_pool */
struct h2_bucket_beam *beam_in; /* c2: data in or NULL, borrowed from request stream */
- apr_pool_t *mplx_pool; /* c2: an mplx child pool for safe use inside mplx lock */
apr_file_t *pipe_in_prod[2]; /* c2: input produced notification pipe */
- apr_file_t *pipe_in_drain[2]; /* c2: input drained notification pipe */
apr_file_t *pipe_out_prod[2]; /* c2: output produced notification pipe */
- apr_pollfd_t pfd_in_drain; /* c2: poll pipe_in_drain output */
apr_pollfd_t pfd_out_prod; /* c2: poll pipe_out_prod output */
int has_final_response; /* final HTTP response passed on out */
apr_status_t last_err; /* APR_SUCCES or last error encountered in filters */
- struct h2_response_parser *parser; /* optional parser to catch H1 responses */
- volatile int done; /* c2: processing has finished */
+ apr_uint32_t started; /* c2: processing was started */
apr_time_t started_at; /* c2: when processing started */
+ apr_uint32_t done; /* c2: processing has finished */
Review comment:
good idea, will add the `/* volatile */` comments.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@httpd.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@httpd.apache.org
For additional commands, e-mail: notifications-help@httpd.apache.org
[GitHub] [httpd] icing commented on a change in pull request #297: H2 pollset impr
Posted by GitBox <gi...@apache.org>.
icing commented on a change in pull request #297:
URL: https://github.com/apache/httpd/pull/297#discussion_r829057364
##########
File path: modules/http2/h2_workers.h
##########
@@ -38,19 +38,17 @@ struct h2_workers {
int next_worker_id;
apr_uint32_t max_workers;
- volatile apr_uint32_t min_workers; /* is changed during graceful shutdown */
- volatile apr_interval_time_t max_idle_duration; /* is changed during graceful shutdown */
-
- volatile int aborted;
- volatile int shutdown;
+ apr_uint32_t min_workers;
+ apr_uint32_t worker_count;
+ apr_uint32_t max_idle_secs;
+ apr_uint32_t aborted;
+ apr_uint32_t shutdown;
Review comment:
ack.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@httpd.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@httpd.apache.org
For additional commands, e-mail: notifications-help@httpd.apache.org
[GitHub] [httpd] icing commented on a change in pull request #297: H2 pollset impr
Posted by GitBox <gi...@apache.org>.
icing commented on a change in pull request #297:
URL: https://github.com/apache/httpd/pull/297#discussion_r829056904
##########
File path: modules/http2/h2_mplx.c
##########
@@ -896,8 +940,8 @@ static void s_c2_done(h2_mplx *m, conn_rec *c2, h2_conn_ctx_t *conn_ctx)
ap_log_cerror(APLOG_MARK, APLOG_TRACE1, 0, c2,
"h2_mplx(%s-%d): c2 done", conn_ctx->id, conn_ctx->stream_id);
- ap_assert(conn_ctx->done == 0);
- conn_ctx->done = 1;
+ AP_DEBUG_ASSERT(apr_atomic_read32(&conn_ctx->done) == 0);
+ apr_atomic_set32(&conn_ctx->done, 1);
Review comment:
A DEBUG_ASSERT() seems best here. It is really only a dev check if things got screwed up by a change. This method needs to be called once and only once for a c2 connection.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@httpd.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@httpd.apache.org
For additional commands, e-mail: notifications-help@httpd.apache.org
[GitHub] [httpd] ylavic commented on a change in pull request #297: H2 pollset impr
Posted by GitBox <gi...@apache.org>.
ylavic commented on a change in pull request #297:
URL: https://github.com/apache/httpd/pull/297#discussion_r829068840
##########
File path: modules/http2/h2_conn_ctx.h
##########
@@ -48,20 +50,17 @@ struct h2_conn_ctx_t {
struct h2_bucket_beam *beam_out; /* c2: data out, created from req_pool */
struct h2_bucket_beam *beam_in; /* c2: data in or NULL, borrowed from request stream */
- apr_pool_t *mplx_pool; /* c2: an mplx child pool for safe use inside mplx lock */
apr_file_t *pipe_in_prod[2]; /* c2: input produced notification pipe */
- apr_file_t *pipe_in_drain[2]; /* c2: input drained notification pipe */
apr_file_t *pipe_out_prod[2]; /* c2: output produced notification pipe */
- apr_pollfd_t pfd_in_drain; /* c2: poll pipe_in_drain output */
apr_pollfd_t pfd_out_prod; /* c2: poll pipe_out_prod output */
int has_final_response; /* final HTTP response passed on out */
apr_status_t last_err; /* APR_SUCCES or last error encountered in filters */
- struct h2_response_parser *parser; /* optional parser to catch H1 responses */
- volatile int done; /* c2: processing has finished */
+ apr_uint32_t started; /* c2: processing was started */
apr_time_t started_at; /* c2: when processing started */
+ apr_uint32_t done; /* c2: processing has finished */
Review comment:
+1
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@httpd.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@httpd.apache.org
For additional commands, e-mail: notifications-help@httpd.apache.org
[GitHub] [httpd] icing commented on a change in pull request #297: H2 pollset impr
Posted by GitBox <gi...@apache.org>.
icing commented on a change in pull request #297:
URL: https://github.com/apache/httpd/pull/297#discussion_r829053788
##########
File path: modules/http2/h2_mplx.c
##########
@@ -254,18 +314,20 @@ h2_mplx *h2_mplx_c1_create(h2_stream *stream0, server_rec *s, apr_pool_t *parent
m->streams_ev_in = apr_array_make(m->pool, 10, sizeof(h2_stream*));
m->streams_ev_out = apr_array_make(m->pool, 10, sizeof(h2_stream*));
-#if !H2_POLL_STREAMS
+ m->streams_input_read = h2_iq_create(m->pool, 10);
status = apr_thread_mutex_create(&m->poll_lock, APR_THREAD_MUTEX_DEFAULT,
m->pool);
if (APR_SUCCESS != status) goto failure;
- m->streams_input_read = h2_iq_create(m->pool, 10);
+#if !H2_POLL_STREAMS
Review comment:
I thought about this. We have support for disabling this bc Windows and older APR versions.
The possible benefit of the file descriptors is that we might adapt `mod_proxy` modules to use them for shuffling content back and forth, as you did for the wstunnel.
But then again, as long as we do not have that on all platforms we support, this means different code paths and complexity.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@httpd.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@httpd.apache.org
For additional commands, e-mail: notifications-help@httpd.apache.org
[GitHub] [httpd] icing commented on a change in pull request #297: H2 pollset impr
Posted by GitBox <gi...@apache.org>.
icing commented on a change in pull request #297:
URL: https://github.com/apache/httpd/pull/297#discussion_r829130863
##########
File path: server/connection.c
##########
@@ -66,6 +70,13 @@ AP_DECLARE(conn_rec *) ap_create_connection(apr_pool_t *p,
return c;
}
+AP_DECLARE(conn_rec *) ap_create_secondary_connection(apr_pool_t *p,
+ conn_rec *master,
+ apr_bucket_alloc_t *alloc)
+{
+ return ap_run_create_secondary_connection(p, master, alloc);
+}
Review comment:
Not opposed to it either. Maybe @rpluem can arbitrate...
##########
File path: modules/http2/h2_mplx.c
##########
@@ -173,6 +176,63 @@ static void m_stream_cleanup(h2_mplx *m, h2_stream *stream)
}
}
+static h2_c2_transit *c2_transit_create(h2_mplx *m)
+{
+ apr_allocator_t *allocator;
+ apr_pool_t *ptrans;
+ h2_c2_transit *transit;
+ apr_status_t rv;
+
+ /* We create a pool with its own allocator to be used for
+ * processing a request. This is the only way to have the processing
+ * independent of its parent pool in the sense that it can work in
+ * another thread.
+ */
+
+ apr_allocator_create(&allocator);
Review comment:
Added the checks. Thanks.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@httpd.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@httpd.apache.org
For additional commands, e-mail: notifications-help@httpd.apache.org
[GitHub] [httpd] ylavic commented on a change in pull request #297: H2 pollset impr
Posted by GitBox <gi...@apache.org>.
ylavic commented on a change in pull request #297:
URL: https://github.com/apache/httpd/pull/297#discussion_r829032875
##########
File path: modules/http2/h2_workers.c
##########
@@ -40,7 +40,7 @@ struct h2_slot {
apr_thread_t *thread;
apr_thread_mutex_t *lock;
apr_thread_cond_t *not_idle;
- volatile apr_uint32_t timed_out;
+ apr_uint32_t timed_out;
Review comment:
`volative` (or `/* volatile */`) might be documentary still
##########
File path: modules/http2/h2_workers.c
##########
@@ -127,10 +127,9 @@ static void wake_idle_worker(h2_workers *workers)
{
h2_slot *slot = pop_slot(&workers->idle);
if (slot) {
- int timed_out = 0;
+ int timed_out;
Review comment:
It looks like `wake_idle_worker()` could use a loop like:
```
for (;;) {
slot = pop_slot());
if (!slot) {
if (workers->dynamic && ...) {
...
}
break;
}
lock();
timed_out = ...;
if (!timed_out) {
signal();
unlock();
break;
}
unlock();
slot_done();
}
```
rather than a recursion?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@httpd.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@httpd.apache.org
For additional commands, e-mail: notifications-help@httpd.apache.org
[GitHub] [httpd] ylavic commented on a change in pull request #297: H2 pollset impr
Posted by GitBox <gi...@apache.org>.
ylavic commented on a change in pull request #297:
URL: https://github.com/apache/httpd/pull/297#discussion_r829032477
##########
File path: modules/http2/h2_mplx.c
##########
@@ -896,8 +940,8 @@ static void s_c2_done(h2_mplx *m, conn_rec *c2, h2_conn_ctx_t *conn_ctx)
ap_log_cerror(APLOG_MARK, APLOG_TRACE1, 0, c2,
"h2_mplx(%s-%d): c2 done", conn_ctx->id, conn_ctx->stream_id);
- ap_assert(conn_ctx->done == 0);
- conn_ctx->done = 1;
+ AP_DEBUG_ASSERT(apr_atomic_read32(&conn_ctx->done) == 0);
+ apr_atomic_set32(&conn_ctx->done, 1);
Review comment:
TOCTOU, should this be:
```
if (apr_atomic_cas32(&conn_ctx->done, 1, 0) != 0) {
APR_DEBUG_ASSERT(0);
}
```
?
And maybe `return;` if already set too? It looks like `s_c2_done()` is not supposed to be called multiple times and if it happens httpd's teeth will fall out (or can we simply ignore it)?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@httpd.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@httpd.apache.org
For additional commands, e-mail: notifications-help@httpd.apache.org
[GitHub] [httpd] ylavic commented on a change in pull request #297: H2 pollset impr
Posted by GitBox <gi...@apache.org>.
ylavic commented on a change in pull request #297:
URL: https://github.com/apache/httpd/pull/297#discussion_r829066156
##########
File path: modules/http2/h2_mplx.c
##########
@@ -254,18 +314,20 @@ h2_mplx *h2_mplx_c1_create(h2_stream *stream0, server_rec *s, apr_pool_t *parent
m->streams_ev_in = apr_array_make(m->pool, 10, sizeof(h2_stream*));
m->streams_ev_out = apr_array_make(m->pool, 10, sizeof(h2_stream*));
-#if !H2_POLL_STREAMS
+ m->streams_input_read = h2_iq_create(m->pool, 10);
status = apr_thread_mutex_create(&m->poll_lock, APR_THREAD_MUTEX_DEFAULT,
m->pool);
if (APR_SUCCESS != status) goto failure;
- m->streams_input_read = h2_iq_create(m->pool, 10);
+#if !H2_POLL_STREAMS
Review comment:
Oh, I wasn't thinking of removing file descriptors but wondered how you managed to remove the `H2_POLL_STREAMS` support for `streams_input_read` and why you couldn't do the same for the output side.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@httpd.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@httpd.apache.org
For additional commands, e-mail: notifications-help@httpd.apache.org
[GitHub] [httpd] icing commented on a change in pull request #297: H2 pollset impr
Posted by GitBox <gi...@apache.org>.
icing commented on a change in pull request #297:
URL: https://github.com/apache/httpd/pull/297#discussion_r829057986
##########
File path: server/connection.c
##########
@@ -66,6 +70,13 @@ AP_DECLARE(conn_rec *) ap_create_connection(apr_pool_t *p,
return c;
}
+AP_DECLARE(conn_rec *) ap_create_secondary_connection(apr_pool_t *p,
+ conn_rec *master,
+ apr_bucket_alloc_t *alloc)
+{
+ return ap_run_create_secondary_connection(p, master, alloc);
+}
Review comment:
I think it is good to have and at no cost. We have added wrappers around hooks in the past, when things turned out to need global checks or additional things.
##########
File path: server/core.c
##########
@@ -5517,6 +5519,55 @@ static conn_rec *core_create_conn(apr_pool_t *ptrans, server_rec *s,
return c;
}
+static conn_rec *core_create_secondary_conn(apr_pool_t *ptrans,
+ conn_rec *master,
+ apr_bucket_alloc_t *alloc)
+{
+ apr_pool_t *pool;
+ conn_config_t *conn_config;
+ conn_rec *c = (conn_rec *) apr_pcalloc(ptrans, sizeof(conn_rec));
+
+ /* Got a connection structure, so initialize what fields we can
+ * (the rest are zeroed out by pcalloc).
+ */
+ apr_pool_create(&pool, ptrans);
+ apr_pool_tag(pool, "secondary_conn");
+
+ /* first copy everything, then replace what is needed */
+ memcpy(c, master, sizeof(*c));
Review comment:
good point.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@httpd.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@httpd.apache.org
For additional commands, e-mail: notifications-help@httpd.apache.org
[GitHub] [httpd] ylavic commented on a change in pull request #297: H2 pollset impr
Posted by GitBox <gi...@apache.org>.
ylavic commented on a change in pull request #297:
URL: https://github.com/apache/httpd/pull/297#discussion_r829063031
##########
File path: server/connection.c
##########
@@ -66,6 +70,13 @@ AP_DECLARE(conn_rec *) ap_create_connection(apr_pool_t *p,
return c;
}
+AP_DECLARE(conn_rec *) ap_create_secondary_connection(apr_pool_t *p,
+ conn_rec *master,
+ apr_bucket_alloc_t *alloc)
+{
+ return ap_run_create_secondary_connection(p, master, alloc);
+}
Review comment:
Yeah right but if at that time for instance we need more parameters in the helper than in the hook we'd have to create and _ex() version if the helper exists already.
Not a strong opinion though..
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@httpd.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@httpd.apache.org
For additional commands, e-mail: notifications-help@httpd.apache.org