apache / incubator-pagespeed-ngx

Automatic PageSpeed optimization module for Nginx
http://ngxpagespeed.com/
Apache License 2.0
4.37k stars 362 forks source link

Unable to see Link header after enabling hint_preload_subresources #1408

Open PikachuEXE opened 7 years ago

PikachuEXE commented 7 years ago

Description

I am unable to see Link headers after enabling hint_preload_subresources in nginx (compiled with ngx_pagespeed) Even the example page inside the doc for this filter does not contain any Link header in response https://modpagespeed.com/doc/filter-hint-preload-subresources

Env

ngx_pagespeed: v1.12.34.2-beta Nginx: 1.11.13

Nginx build config:

nginx version: nginx/1.11.13
built by gcc 5.4.0 20160609 (Ubuntu 5.4.0-6ubuntu1~16.04.4)
built with OpenSSL 1.1.0e  16 Feb 2017
TLS SNI support enabled
configure arguments: --prefix=/usr/local/nginx --sbin-path=/usr/local/sbin/nginx --conf-path=/etc/nginx/nginx.conf --error-log-path=/var/log/nginx/error.log --http-log-path=/var/log/nginx/access.log --pid-path=/run/nginx.pid --lock-path=/run/lock/subsys/nginx --user=nginx --group=nginx --with-openssl=/usr/src/openssl --add-module=/usr/local/src/ngx_http_geoip2_module --add-module=/usr/local/src/ngx_pagespeed_module --with-file-aio --with-ipv6 --with-http_ssl_module --with-http_v2_module --with-http_realip_module --with-http_addition_module --with-http_xslt_module --with-http_image_filter_module --with-http_geoip_module --with-http_sub_module --with-http_dav_module --with-http_flv_module --with-http_mp4_module --with-http_gunzip_module --with-http_gzip_static_module --with-http_random_index_module --with-http_secure_link_module --with-http_degradation_module --with-http_stub_status_module --with-http_perl_module --with-mail --with-mail_ssl_module --with-pcre --with-google_perftools_module

Page Speed Config in Nginx config file

    # === PageSpeed === #
    #
    # https://modpagespeed.com/doc/configuration

    pagespeed on;

    # Needs to exist and be writable by nginx.  Use tmpfs for best performance.
    pagespeed FileCachePath /var/cache/ngx_pagespeed/;

    # Ensure requests for pagespeed optimized resources go to the pagespeed handler
    # and no extraneous headers get set.
    location ~ "\.pagespeed\.([a-z]\.)?[a-z]{2}\.[^.]{10}\.[^.]+" {
      add_header "" "";
    }
    location ~ "^/pagespeed_static/" { }
    location ~ "^/ngx_pagespeed_beacon$" { }

    # Ensure relative URLs not to be rewritten to absolute URLs
    pagespeed PreserveUrlRelativity on;

    # Better performance with lowercase keywords in HTML
    pagespeed LowercaseHtmlNames on;

    # Remove extra whitespaces in HTML documents
    # Safe to be turned on as long as CSS property `white-space` unused
    pagespeed EnableFilters collapse_whitespace;

    # Change
    # <button name="ok" disabled="disabled">
    # to
    # <button name="ok" disabled>
    pagespeed EnableFilters elide_attributes;

    # Add resource preloading HTTP headers
    #
    # Spec: http://w3c.github.io/preload/
    # Browser Support: http://caniuse.com/#search=preload
    pagespeed EnableFilters hint_preload_subresources;

    # === PageSpeed === #
oschaaf commented 7 years ago

Confirmed on master. Initially the responses look right. Then after a while, the Link: headers disappear from the responses and do not return again. Restarting nginx does not resolve the problem, but purging the cache does.

oschaaf commented 7 years ago

In the flow where the Link: headers are not written, the following code line is hit: https://github.com/pagespeed/mod_pagespeed/blob/master/net/instaweb/rewriter/push_preload_filter.cc#L95 Which has the comment:

// Stop at first invalid entry, to avoid out-of-order hints.

That explains why the Link: headers are not written, but I'm not sure this line should actually be hit in this situation: I suspect we end up there after property cache entries expire for the page that are used to remember which assets should be preloaded.

In the first pass and the flows where the Link: headers are written this line is not hit.

oschaaf commented 7 years ago

It looks like the first InputInfo associated to the first Dependency object in the set that the code loops in PushPreloadFilter::StartDocumentImpl has expired, and never freshens. This line keeps getting hit: https://github.com/pagespeed/mod_pagespeed/blob/master/net/instaweb/rewriter/input_info_utils.cc#L153

PikachuEXE commented 7 years ago

Anything we can do to help solving this issue? I am Rails dev, can't code (or even read) in C :(

Also dunno how to test it quickly after I manage to "fix" it (got test suite?)

oschaaf commented 7 years ago

@PikachuEXE it would be really helpful if you could confirm the fix once we have one.

To solve this, it looks like the challenge is in figuring out how to correctly trigger revalidation of the cached metadata for the Link: headers once it has become stale.

morlovich commented 7 years ago

In theory it should "just happen" since the filter that computes the prefetch should have exact same invalidation info in its metadata cache entry...

On Tue, Apr 18, 2017 at 6:17 AM, Otto van der Schaaf < notifications@github.com> wrote:

@PikachuEXE https://github.com/PikachuEXE it would be really helpful if you could confirm the fix once we have one.

To solve this, it looks like the challenge is in figuring out how to correctly trigger revalidation of the cached metadata for the Link: headers once it has become stale.

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/pagespeed/ngx_pagespeed/issues/1408#issuecomment-294765693, or mute the thread https://github.com/notifications/unsubscribe-auth/ADl1RHMKL--enWexZibvgCSfO98FHJ5Xks5rxI3XgaJpZM4M8cXe .

coffeebite commented 6 years ago

Any updates on this folks? I am seeing the same behavior.

sebagr commented 6 years ago

+1 still seeing this behavior