You are viewing a plain text version of this content. The canonical link for it is here.
Posted to cvs@httpd.apache.org by ta...@apache.org on 2014/06/25 14:24:04 UTC
svn commit: r1605369 - /httpd/httpd/trunk/modules/proxy/mod_proxy_wstunnel.c
Author: takashi
Date: Wed Jun 25 12:24:03 2014
New Revision: 1605369
URL: http://svn.apache.org/r1605369
Log:
Refactor asynchronous mod_proxy_wstunnel using pollfd returned by MPM.
r1601943 and r1605307 made Event MPM return woken pollfd, so async
wstunnel doesn't need its own apr_pollset_poll.
Modified:
httpd/httpd/trunk/modules/proxy/mod_proxy_wstunnel.c
Modified: httpd/httpd/trunk/modules/proxy/mod_proxy_wstunnel.c
URL: http://svn.apache.org/viewvc/httpd/httpd/trunk/modules/proxy/mod_proxy_wstunnel.c?rev=1605369&r1=1605368&r2=1605369&view=diff
==============================================================================
--- httpd/httpd/trunk/modules/proxy/mod_proxy_wstunnel.c (original)
+++ httpd/httpd/trunk/modules/proxy/mod_proxy_wstunnel.c Wed Jun 25 12:24:03 2014
@@ -38,21 +38,68 @@ typedef struct ws_baton_t {
static apr_status_t proxy_wstunnel_transfer(request_rec *r, conn_rec *c_i, conn_rec *c_o,
apr_bucket_brigade *bb, char *name);
-static void proxy_wstunnel_callback(void *b, const apr_pollfd_t *dummy);
+static void proxy_wstunnel_callback(void *b, const apr_pollfd_t *pfd);
-static int proxy_wstunnel_pump(ws_baton_t *baton, apr_time_t timeout, int try_async) {
+static apr_status_t proxy_wstunnel_pump2(ws_baton_t *baton, const apr_pollfd_t *pfd){
request_rec *r = baton->r;
conn_rec *c = r->connection;
proxy_conn_rec *conn = baton->proxy_connrec;
apr_socket_t *sock = conn->sock;
conn_rec *backconn = conn->connection;
+ apr_int16_t pollevent;
+ apr_socket_t *client_socket = baton->client_soc;
+ apr_bucket_brigade *bb = baton->bb;
+
+ if (pfd->desc.s == sock) {
+ pollevent = pfd->rtnevents;
+ if (pollevent & (APR_POLLIN | APR_POLLHUP)) {
+ ap_log_rerror(APLOG_MARK, APLOG_TRACE1, 0, r, APLOGNO(02446)
+ "sock was readable");
+ return proxy_wstunnel_transfer(r, backconn, c, bb, "sock");
+ }
+ else if (pollevent & APR_POLLERR) {
+ ap_log_rerror(APLOG_MARK, APLOG_NOTICE, 0, r, APLOGNO(02447)
+ "error on backconn");
+ return APR_EPIPE;
+ }
+ else {
+ ap_log_rerror(APLOG_MARK, APLOG_NOTICE, 0, r, APLOGNO(02605)
+ "unknown event on backconn %d", pollevent);
+ return APR_EGENERAL;
+ }
+ }
+ else if (pfd->desc.s == client_socket) {
+ pollevent = pfd->rtnevents;
+ if (pollevent & (APR_POLLIN | APR_POLLHUP)) {
+ ap_log_rerror(APLOG_MARK, APLOG_TRACE1, 0, r, APLOGNO(02448)
+ "client was readable");
+ return proxy_wstunnel_transfer(r, c, backconn, bb, "client");
+ }
+ else if (pollevent & APR_POLLERR) {
+ ap_log_rerror(APLOG_MARK, APLOG_TRACE1, 0, r, APLOGNO(02607)
+ "error on client conn");
+ c->aborted = 1;
+ return APR_EPIPE;
+ }
+ else {
+ ap_log_rerror(APLOG_MARK, APLOG_NOTICE, 0, r, APLOGNO(02606)
+ "unknown event on client conn %d", pollevent);
+ return APR_EGENERAL;
+ }
+ }
+ else {
+ ap_log_rerror(APLOG_MARK, APLOG_INFO, 0, r, APLOGNO(02449)
+ "unknown socket in pollset");
+ return APR_EBADF;
+ }
+}
+
+static int proxy_wstunnel_pump(ws_baton_t *baton, apr_time_t timeout, int try_async) {
+ request_rec *r = baton->r;
const apr_pollfd_t *signalled;
apr_int32_t pollcnt, pi;
- apr_int16_t pollevent;
apr_pollset_t *pollset = baton->pollset;
- apr_socket_t *client_socket = baton->client_soc;
apr_status_t rv;
- apr_bucket_brigade *bb = baton->bb;
while(1) {
if ((rv = apr_pollset_poll(pollset, timeout, &pollcnt, &signalled))
@@ -79,51 +126,7 @@ static int proxy_wstunnel_pump(ws_baton_
"woke from poll(), i=%d", pollcnt);
for (pi = 0; pi < pollcnt; pi++) {
- const apr_pollfd_t *cur = &signalled[pi];
-
- if (cur->desc.s == sock) {
- pollevent = cur->rtnevents;
- if (pollevent & (APR_POLLIN | APR_POLLHUP)) {
- ap_log_rerror(APLOG_MARK, APLOG_TRACE1, 0, r, APLOGNO(02446)
- "sock was readable");
- rv = proxy_wstunnel_transfer(r, backconn, c, bb, "sock");
- }
- else if (pollevent & APR_POLLERR) {
- rv = APR_EPIPE;
- ap_log_rerror(APLOG_MARK, APLOG_NOTICE, 0, r, APLOGNO(02447)
- "error on backconn");
- }
- else {
- rv = APR_EGENERAL;
- ap_log_rerror(APLOG_MARK, APLOG_NOTICE, 0, r, APLOGNO(02605)
- "unknown event on backconn %d", pollevent);
- }
- }
- else if (cur->desc.s == client_socket) {
- pollevent = cur->rtnevents;
- if (pollevent & (APR_POLLIN | APR_POLLHUP)) {
- ap_log_rerror(APLOG_MARK, APLOG_TRACE1, 0, r, APLOGNO(02448)
- "client was readable");
- rv = proxy_wstunnel_transfer(r, c, backconn, bb, "client");
- }
- else if (pollevent & APR_POLLERR) {
- rv = APR_EPIPE;
- c->aborted = 1;
- ap_log_rerror(APLOG_MARK, APLOG_TRACE1, 0, r, APLOGNO(02607)
- "error on client conn");
- }
- else {
- rv = APR_EGENERAL;
- ap_log_rerror(APLOG_MARK, APLOG_NOTICE, 0, r, APLOGNO(02606)
- "unknown event on client conn %d", pollevent);
- }
- }
- else {
- rv = APR_EBADF;
- ap_log_rerror(APLOG_MARK, APLOG_INFO, 0, r, APLOGNO(02449)
- "unknown socket in pollset");
- }
-
+ rv = proxy_wstunnel_pump2(baton, &signalled[pi]);
}
if (rv != APR_SUCCESS) {
break;
@@ -164,14 +167,12 @@ static void proxy_wstunnel_cancel_callba
* We don't need the invoke_mtx, since we never put multiple callback events
* in the queue.
*/
-static void proxy_wstunnel_callback(void *b, const apr_pollfd_t *dummy) {
- int status;
+static void proxy_wstunnel_callback(void *b, const apr_pollfd_t *pfd) {
apr_socket_t *sockets[3] = {NULL, NULL, NULL};
ws_baton_t *baton = (ws_baton_t*)b;
proxyws_dir_conf *dconf = ap_get_module_config(baton->r->per_dir_config, &proxy_wstunnel_module);
apr_pool_clear(baton->subpool);
- status = proxy_wstunnel_pump(baton, dconf->async_delay, dconf->is_async);
- if (status == SUSPENDED) {
+ if (proxy_wstunnel_pump2(baton, pfd) == APR_SUCCESS) {
sockets[0] = baton->client_soc;
sockets[1] = baton->server_soc;
ap_mpm_register_socket_callback_timeout(sockets, baton->subpool, 1,
Re: svn commit: r1605369 - /httpd/httpd/trunk/modules/proxy/mod_proxy_wstunnel.c
Posted by Takashi Sato <ta...@tks.st>.
Reverted as r1605946
Re: svn commit: r1605369 - /httpd/httpd/trunk/modules/proxy/mod_proxy_wstunnel.c
Posted by Jim Jagielski <ji...@jaguNET.com>.
+1 for reverting for now.
On Jun 26, 2014, at 2:41 AM, Takashi Sato <ta...@tks.st> wrote:
> 2014-06-26 12:34 GMT+09:00 Eric Covener <co...@gmail.com>:
> [cut]
>> I think this is only a performance concern.
>
> I think many things and yes, I realize maybe performance can be
> worse than before in both socket readable case.
>
> Before:
> poll (event MPM)
> pollset_remove
> poll (proxy_wstunnel)
> read/write (1st socket)
> read/write (2nd socket)
> pollset_add
>
> After:
> poll (event MPM)
> pollset_remove
> read/write (1st socket)
> pollset_add
> poll (event MPM)
> pollset_remove
> read/write (2nd socket)
> pollset_add
>
> I intend to do either following:
>
> 1) revert this.
> I also doubt if r1601943 and r1605307 is useful.
>
> 2) make event MPM return all woken pollfd to wstunnel
> This is not very hard work, but additional code complexity.
> I don't think this is worth adding.
>
Re: svn commit: r1605369 - /httpd/httpd/trunk/modules/proxy/mod_proxy_wstunnel.c
Posted by Takashi Sato <ta...@tks.st>.
2014-06-26 12:34 GMT+09:00 Eric Covener <co...@gmail.com>:
[cut]
> I think this is only a performance concern.
I think many things and yes, I realize maybe performance can be
worse than before in both socket readable case.
Before:
poll (event MPM)
pollset_remove
poll (proxy_wstunnel)
read/write (1st socket)
read/write (2nd socket)
pollset_add
After:
poll (event MPM)
pollset_remove
read/write (1st socket)
pollset_add
poll (event MPM)
pollset_remove
read/write (2nd socket)
pollset_add
I intend to do either following:
1) revert this.
I also doubt if r1601943 and r1605307 is useful.
2) make event MPM return all woken pollfd to wstunnel
This is not very hard work, but additional code complexity.
I don't think this is worth adding.
Re: svn commit: r1605369 - /httpd/httpd/trunk/modules/proxy/mod_proxy_wstunnel.c
Posted by Christophe JAILLET <ch...@wanadoo.fr>.
Le 27/06/2014 06:44, Christophe JAILLET a écrit :
> Le 26/06/2014 05:34, Eric Covener a écrit :
>> I think this is only a performance concern.
>
> Hi,
>
> I have on my local tree an attempt to speed-up wstunnel by removing
> useless processing mostly for non-EBCDIC server.
>
> The idea, taken from mod_proxy_http, is to:
> - avoid a call to 'apr_pstrcat' to built a constant string
> - avoid calls to 'strlen' (even if gcc should be able to remove
> one of them at compile time)
>
> I tried to go one step further, for non-EBCDIC server, and use an
> immortal bucket instead of the pool one. This avoids the allocation of
> a few bytes in a memory pool and saves a memcpy.
>
> Obviously, this is done only once per-connection when the request is
> upgraded to WS.
>
>
> This is untested and I was wondering if it would make any difference
> in the real world?
> Do you think it worses the added complexity ?
>
>
> The same kind of approach can also be done in mod_proxy_http in order
> to avoid a memory allocation and a strcpy.
>
> Best regards,
> CJ
With a non-empty file, it's better !
CJ
Re: svn commit: r1605369 - /httpd/httpd/trunk/modules/proxy/mod_proxy_wstunnel.c
Posted by Christophe JAILLET <ch...@wanadoo.fr>.
Le 26/06/2014 05:34, Eric Covener a écrit :
> I think this is only a performance concern.
Hi,
I have on my local tree an attempt to speed-up wstunnel by removing
useless processing mostly for non-EBCDIC server.
The idea, taken from mod_proxy_http, is to:
- avoid a call to 'apr_pstrcat' to built a constant string
- avoid calls to 'strlen' (even if gcc should be able to remove one
of them at compile time)
I tried to go one step further, for non-EBCDIC server, and use an
immortal bucket instead of the pool one. This avoids the allocation of a
few bytes in a memory pool and saves a memcpy.
Obviously, this is done only once per-connection when the request is
upgraded to WS.
This is untested and I was wondering if it would make any difference in
the real world?
Do you think it worses the added complexity ?
The same kind of approach can also be done in mod_proxy_http in order to
avoid a memory allocation and a strcpy.
Best regards,
CJ
Re: svn commit: r1605369 - /httpd/httpd/trunk/modules/proxy/mod_proxy_wstunnel.c
Posted by Eric Covener <co...@gmail.com>.
On Wed, Jun 25, 2014 at 11:23 PM, Takashi Sato <ta...@tks.st> wrote:
> I can't say it is really really safe now.
> But IMHO if it is not safe, I think we should fix, modify and enhance
> the MPM. MPMs should guarantee such safety.
> I will read and test event.c more and want to be sure.
I assumed functionally it is safe either way, because the pollset is
level-triggered and we'll always put both sockets right back in, we
can't miss an event.
I think this is only a performance concern.
--
Eric Covener
covener@gmail.com
Re: svn commit: r1605369 - /httpd/httpd/trunk/modules/proxy/mod_proxy_wstunnel.c
Posted by Takashi Sato <ta...@tks.st>.
>>> If both sockets are readable, we'll go all the way through
>>> ap_mpm_register_socket_callback_timeout + locking queues and getting
>>> dispatched to a new thread to read the 2nd socket.
>>
>> So, you think the new code is poor performance than before?
>> Or are you afraid that the 1st and 2nd socket are proceed at the same time?
>> The latter doesn't happen, because event.c line 1968:
>>
>> /* We only signal once per N sockets with this baton */
>> if (!(baton->signaled)) {
>>
>> so only the 1st socket is sent to worker thread.
>
> I think hopping off that thread during a flurry of websockets activity
> could hurt but I can't say for sure.
I can't say it is really really safe now.
But IMHO if it is not safe, I think we should fix, modify and enhance
the MPM. MPMs should guarantee such safety.
I will read and test event.c more and want to be sure.
In the future I want to make more modules async.
(mod_proxy_* and mod_cgid)
> From the context of the diff -- maybe if it's non-zero (non default),
> we could use the old path directly.
I tried.
At first ProxyWebsocketAsyncDelay is effective, but after go async
(trace1 log "AH02542: Attempting to go async"),
ProxyWebsocketAsyncDelay is ignored.
I guess making it effective again is easy, maybe I have to add
a few code.
But I still doubt whether it is useful for users, though it may be
useful for us for debugging or developing.
Re: svn commit: r1605369 - /httpd/httpd/trunk/modules/proxy/mod_proxy_wstunnel.c
Posted by Eric Covener <co...@gmail.com>.
On Wed, Jun 25, 2014 at 12:55 PM, Takashi Sato <ta...@tks.st> wrote:
> 2014-06-25 21:33 GMT+09:00 Eric Covener <co...@gmail.com>:
>> On Wed, Jun 25, 2014 at 8:24 AM, <ta...@apache.org> wrote:
>>> Refactor asynchronous mod_proxy_wstunnel using pollfd returned by MPM.
>>> r1601943 and r1605307 made Event MPM return woken pollfd, so async
>>> wstunnel doesn't need its own apr_pollset_poll.
>>
>> If both sockets are readable, we'll go all the way through
>> ap_mpm_register_socket_callback_timeout + locking queues and getting
>> dispatched to a new thread to read the 2nd socket.
>
> So, you think the new code is poor performance than before?
> Or are you afraid that the 1st and 2nd socket are proceed at the same time?
> The latter doesn't happen, because event.c line 1968:
>
> /* We only signal once per N sockets with this baton */
> if (!(baton->signaled)) {
>
> so only the 1st socket is sent to worker thread.
I think hopping off that thread during a flurry of websockets activity
could hurt but I can't say for sure.
>
>> I think this also prevents us from doing ProxyWebsocketAsyncDelay to
>> stay on the thread because we no longer have a poll during the
>> callback invocation.
>
> Yes, you are right, but, is ProxyWebsocketAsyncDelay needed?
I don't know. It was more or less free at the time, and I was more
anticipating what knobs might be useful.
>From the context of the diff -- maybe if it's non-zero (non default),
we could use the old path directly.
--
Eric Covener
covener@gmail.com
Re: svn commit: r1605369 - /httpd/httpd/trunk/modules/proxy/mod_proxy_wstunnel.c
Posted by Takashi Sato <ta...@tks.st>.
2014-06-25 21:33 GMT+09:00 Eric Covener <co...@gmail.com>:
> On Wed, Jun 25, 2014 at 8:24 AM, <ta...@apache.org> wrote:
>> Refactor asynchronous mod_proxy_wstunnel using pollfd returned by MPM.
>> r1601943 and r1605307 made Event MPM return woken pollfd, so async
>> wstunnel doesn't need its own apr_pollset_poll.
>
> If both sockets are readable, we'll go all the way through
> ap_mpm_register_socket_callback_timeout + locking queues and getting
> dispatched to a new thread to read the 2nd socket.
So, you think the new code is poor performance than before?
Or are you afraid that the 1st and 2nd socket are proceed at the same time?
The latter doesn't happen, because event.c line 1968:
/* We only signal once per N sockets with this baton */
if (!(baton->signaled)) {
so only the 1st socket is sent to worker thread.
> I think this also prevents us from doing ProxyWebsocketAsyncDelay to
> stay on the thread because we no longer have a poll during the
> callback invocation.
Yes, you are right, but, is ProxyWebsocketAsyncDelay needed?
Re: svn commit: r1605369 - /httpd/httpd/trunk/modules/proxy/mod_proxy_wstunnel.c
Posted by Eric Covener <co...@gmail.com>.
On Wed, Jun 25, 2014 at 8:24 AM, <ta...@apache.org> wrote:
> Refactor asynchronous mod_proxy_wstunnel using pollfd returned by MPM.
> r1601943 and r1605307 made Event MPM return woken pollfd, so async
> wstunnel doesn't need its own apr_pollset_poll.
If both sockets are readable, we'll go all the way through
ap_mpm_register_socket_callback_timeout + locking queues and getting
dispatched to a new thread to read the 2nd socket.
I think this also prevents us from doing ProxyWebsocketAsyncDelay to
stay on the thread because we no longer have a poll during the
callback invocation.
--
Eric Covener
covener@gmail.com