> -----Original Message-----
> From: Jeff Kaufman [mailto:jef...@google.com.INVALID]
> Sent: donderdag 29 oktober 2015 21:13
> To: dev@serf.apache.org
> Cc: Jeffrey Crowell <jcrow...@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;
> }
