serf-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Bert Huijben" <b...@qqmail.nl>
Subject RE: Some PageSpeed changes
Date Thu, 29 Oct 2015 20:53:52 GMT
> -----Original Message-----
> From: Jeff Kaufman [mailto:jefftk@google.com.INVALID]
> Sent: donderdag 29 oktober 2015 21:13
> To: dev@serf.apache.org
> Cc: Jeffrey Crowell <jcrowell@google.com>
> Subject: Some PageSpeed changes
> 
> PageSpeed (mod_pagespeed, ngx_pagespeed) uses serf as its http(s)
> client and maintains some patches.  We had a bunch of patches against
> 1.1.0, and now have somewhat fewer with 1.3.8, but I wanted to check
> with you to see if you'd be interested in adopting any of these
> upstream.
> 
> 1. In response_buckets.c, when Content-Length and Transfer-Encoding
> are both present serf goes with Content-Length.  The HTTP spec says
> Transfer-Encoding takes precedence here, and our experience with
> servers has been that if both are present then Transfer-Encoding is
> what we should actually pay attention to.  We patched this to change:
> 
>             v = serf_bucket_headers_get(ctx->headers, "Content-Length");
>             if (v) {
>                 apr_uint64_t length;
>                 length = apr_strtoi64(v, NULL, 10);
>                 if (errno == ERANGE) {
>                     return APR_FROM_OS_ERROR(ERANGE);
>                 }
>                 ctx->body = serf_bucket_response_body_create(
>                               ctx->body, length, bkt->allocator);
>             }
>             else {
>                 v = serf_bucket_headers_get(ctx->headers, "Transfer-Encoding");
> 
>                 /* Need to handle multiple transfer-encoding. */
>                 if (v && strcasecmp("chunked", v) == 0) {
>                     ctx->chunked = 1;
>                     ctx->body = serf_bucket_dechunk_create(ctx->body,
>                                                            bkt->allocator);
>                 }
>             }
> 
> to
> 
>             v = serf_bucket_headers_get(ctx->headers, "Transfer-Encoding");
>             if (v) {
>                 /* Need to handle multiple transfer-encoding. */
>                 if (v && strcasecmp("chunked", v) == 0) {
>                     ctx->chunked = 1;
>                     ctx->body = serf_bucket_dechunk_create(ctx->body,
>                                                            bkt->allocator);
>                 }
>             }
>             else {
>                v = serf_bucket_headers_get(ctx->headers, "Content-Length");
>                if (v) {
>                   apr_uint64_t length;
>                   length = apr_strtoi64(v, NULL, 10);
>                   if (errno == ERANGE) {
>                       return APR_FROM_OS_ERROR(ERANGE);
>                   }
>                   ctx->body = serf_bucket_response_body_create(
>                                  ctx->body, length, bkt->allocator);
>                }
>             }

I think we should apply this change. I recently noticed this in the updated HTTP/1.1 specs
too.

I think both if-s in the initial part can be combined to allow falling back to content-length
if transfer encoding is set, but not 'chunked'. If I remember the spec correctly it can even
have multiple values.
(Spending too much time in the spec for the last few days :-))

> 2. In response_buckets.c serf automatically uncompresses responses,
> but PageSpeed would rather receive them compressed if they're served
> that way.  Currently we just remove the block:
> 
>             v = serf_bucket_headers_get(ctx->headers, "Content-Encoding");
>             if (v) {
>                 /* Need to handle multiple content-encoding. */
>                 if (v && strcasecmp("gzip", v) == 0) {
>                     ctx->body =
>                         serf_bucket_deflate_create(ctx->body, bkt->allocator,
>                                                    SERF_DEFLATE_GZIP);
>                 }
>                 else if (v && strcasecmp("deflate", v) == 0) {
>                     ctx->body =
>                         serf_bucket_deflate_create(ctx->body, bkt->allocator,
>                                                    SERF_DEFLATE_DEFLATE);
>                 }
>             }
> 
> Ideally, though, whether to uncompress would be a serf setting.

A new setting can't be backported to 1.3.x, but I don't see why this can't be configurable
in 1.4+

> 3. We want to be able to check if a connection had error events
> reported to it during the last call to serf_context_run so that we can
> manually close it and open a new connection.  We added a new API call
> to outgoing.c to give this information:
> 
>     int serf_connection_is_in_error_state(serf_connection_t* conn)
>     {
>       return ((conn->seen_in_pollset & (APR_POLLERR | APR_POLLHUP)) !=
> 0);
>     }

I don't think this is really going to work. There are more cases where a connection is in
an error state than just these events. Some platforms report connection errors in a different
way...

Why can't you use serf's own connection resetting? In Subversion we just rely on Serf resetting
connections as needed.

 
> 4. If you're a CDN, one setup is for someone to CNAME www.example.com
> to you and point ref.example.com at their origin.  Then you want to
> fetch from ref.example.com but you need to send "www.example.com" as
> the HOST header.  So we'd like to set the HOST header separately on
> requests.  We added serf_request_bucket_request_create_for_host for
> this:

I'm not entirely sure what you try to accomplish here.

Do you want to send requests to a different host than that you connected to... or explicitly
send a 'wrong' host?

I would guess that you can just change 'Host' in the setup or request handler of the request?
Have you tried that approach?

It is probably more useful to fix that case (if necessary) than to add a separate api for
this corner case.

> -serf_bucket_t *serf_request_bucket_request_create(
> +/*
> + * PageSpeed customization: Add
> serf_request_bucket_request_create_for_host
> + * which lets Host: be set separately from the URL.
> + */
> +serf_bucket_t *serf_request_bucket_request_create_for_host(
>      serf_request_t *request,
>      const char *method,
>      const char *uri,
>      serf_bucket_t *body,
> -    serf_bucket_alloc_t *allocator)
> +    serf_bucket_alloc_t *allocator,
> +    const char *host)
>  {
>      serf_bucket_t *req_bkt, *hdrs_bkt;
>      serf_connection_t *conn = request->conn;
> @@ -1697,9 +1724,10 @@
>          serf_bucket_request_set_root(req_bkt, conn->host_url);
>      }
> 
> -    if (conn->host_info.hostinfo)
> -        serf_bucket_headers_setn(hdrs_bkt, "Host",
> -                                 conn->host_info.hostinfo);
> +    if (host == NULL)
> +      host = request->conn->host_info.hostname;
> +    if (host)
> +        serf_bucket_headers_setn(hdrs_bkt, "Host", host);
> 
>      /* Setup server authorization headers, unless this is a CONNECT request.
> */
>      if (!request->ssltunnel) {
> @@ -1732,6 +1760,17 @@
>      return req_bkt;
>  }
> 
> +serf_bucket_t *serf_request_bucket_request_create(
> +    serf_request_t *request,
> +    const char *method,
> +    const char *uri,
> +    serf_bucket_t *body,
> +    serf_bucket_alloc_t *allocator)
> +{
> +  return serf_request_bucket_request_create_for_host(
> +      request, method, uri, body, allocator, NULL);
> +}
> 
> 5.  We want to be able to set a custom certificates directory or file,
> so we added an API to ssl_buckets.c for that:
> 
> +apr_status_t serf_ssl_set_certificates_directory(serf_ssl_context_t
> *ssl_ctx,
> +                                                 const char* path)
> +{
> +    X509_STORE *store = SSL_CTX_get_cert_store(ssl_ctx->ctx);
> +    int result = X509_STORE_load_locations(store, NULL, path);
> +    return result ? APR_SUCCESS : APR_EGENERAL;
> +}
> +
> +apr_status_t serf_ssl_set_certificates_file(serf_ssl_context_t
> *ssl_ctx,
> +                                             const char* file)
> +{
> +    X509_STORE *store = SSL_CTX_get_cert_store(ssl_ctx->ctx);
> +    int result = X509_STORE_load_locations(store, file, NULL);
> +    return result ? APR_SUCCESS : APR_EGENERAL;
> +}

I'm not sure if this passes the file and path in the right encoding. In general serf follows
apr, which on Windows has a UTF-8 pathstyle, while openssl probably just passes the path to
the ANSI apis on Windows.

We have a serf_ssl_load_cert_file() on trunk that works around these limitations (and certain
openssl linkage problems). We should probably use similar code for these functions.

	Bert

> 6. We null out ctx->ssl_ctx after it's free'd to help make sure it
> doesn't get reused:
> 
>      if (!--ctx->ssl_ctx->refcount) {
>          ssl_free_context(ctx->ssl_ctx);
> +        ctx->ssl_ctx = NULL;
>      }


Mime
View raw message