New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

defer_javascript / js_defer not loaded (0 bytes) when using pagespeed in combination with HTTPS #957

Closed
marcoroest87 opened this Issue Apr 27, 2015 · 60 comments

Comments

Projects
None yet
8 participants
@marcoroest87
Copy link

marcoroest87 commented Apr 27, 2015

When using ngx_pagespeed in combination HTTPS the js_defer.js isn't loaded on the first request (SSL negotiation?)

I'm running nginx 1.7.12 + ngx_pagespeed (master branch, but I tried 1.9.32.3 with the same result) on debian wheezy.

My setup looks like this

  1. Nginx (443) - pagespeed and SSL offloading
  2. Varnish (8081)
    3 Nginx (8080)

This is easily reproducable by using Firefox after hard reloading the page. This can be tested here: https://demo.hypershop.nl/activity-trackers.html (on all other pages the defer_javascript filter is disabled). The issue also occurs using Chrome, but less often is seems.
Since we load the main css with javascript (top css gets included into the html) this results in a page where only the top is shown and no javascript is loaded :(

There are no errors in nginx's error log. (I've tried buidling Nginx with --with-debug option, but this resulted in nginx not being able to server any pages at all).

This is what the access log shows:
94.209.44.170 - - [27/Apr/2015:16:59:36 +0200] "GET /pagespeed_static/1.JiBnMqyl6S.gif HTTP/1.1" 200 0 "https://demo.hypershop.nl/activity-trackers.html" "Mozilla/5.0 (Windows NT 6.1; WOW64; rv:37.0) Gecko/20100101 Firefox/37.0"

Screenshot (Firefox):
https://demo.hypershop.nl/firefox.jpg

This i've tried:

  • using LoadFromFile instead of using FetchHttps
  • Not using SPDY (using SSL instead)
  • tried adding the filters in different orders
  • Using a HTTP proxy like Fiddler. When using Fiddlers ssl certificate in Firefox I cannot reproduce this issue

Config files:

This is how I've build nginx:
./configure
--prefix=/usr/share/nginx
--sbin-path=/usr/sbin/nginx
--conf-path=/etc/nginx/nginx.conf
--pid-path=/var/run/nginx.pid
--lock-path=/var/lock/nginx.lock
--error-log-path=/var/log/nginx/error.log
--http-log-path=/var/log/access.log
--user=www-data
--group=www-data
--without-mail_pop3_module
--without-mail_imap_module
--without-mail_smtp_module
--with-http_stub_status_module
--with-http_ssl_module
--with-http_spdy_module
--with-http_gzip_static_module
--with-http_geoip_module
--with-file-aio
--add-module=$HOME/ngx_pagespeed-master

OpenSSL version:
OpenSSL 1.0.1e 11 Feb 2013
built on: Thu Mar 19 18:31:36 UTC 2015
platform: debian-amd64
options: bn(64,64) rc4(16x,int) des(idx,cisc,16,int) blowfish(idx)
compiler: gcc -fPIC -DOPENSSL_PIC -DZLIB -DOPENSSL_THREADS -D_REENTRANT -DDSO_DLFCN -DHAVE_DLFCN_H -m64 -DL_ENDIAN -DTERMIO -g -O2 -fstack-protector --param=ssp-buffer-size=4 -Wformat -Werror=format-security -D_FORTIFY_SOURCE=2 -Wl,-z,relro -Wa,--noexecstack -Wall -DMD32_REG_T=int -DOPENSSL_IA32_SSE2 -DOPENSSL_BN_ASM_MONT -DOPENSSL_BN_ASM_MONT5 -DOPENSSL_BN_ASM_GF2m -DSHA1_ASM -DSHA256_ASM -DSHA512_ASM -DMD5_ASM -DAES_ASM -DVPAES_ASM -DBSAES_ASM -DWHIRLPOOL_ASM -DGHASH_ASM

@HansVanEijsden

This comment has been minimized.

Copy link

HansVanEijsden commented Apr 27, 2015

I have the same problem on https://www.rtv8.nl - on the first request the slider doesn't load and the "Update Required" message on the right doesn't vanish.
One refresh and all is well.

Let me know if you need more information/details.

./nginx -V

nginx version: nginx/1.7.12
TLS SNI support enabled
configure arguments: --prefix=/opt/nginx17 --user=www-data --group=www-data --with-http_ssl_module --with-http_spdy_module --with-openssl=/usr/local/src/openssl-1.0.2a --with-openssl-opt='enable-ec_nistp_64_gcc_128 threads' --with-md5=/usr/local/src/openssl-1.0.2a --with-md5-asm --with-sha1=/usr/local/src/openssl-1.0.2a --with-sha1-asm --with-pcre-jit --with-file-aio --with-http_flv_module --with-http_geoip_module --with-http_gzip_static_module --with-http_gunzip_module --with-http_mp4_module --with-http_realip_module --with-http_stub_status_module --with-threads --with-ipv6 --add-module=/usr/local/src/nginx-rtmp-module --add-module=/usr/local/src/ngx_cache_purge-2.3 --add-module=/usr/local/src/ngx_http_substitutions_filter_module --add-module=/home/hans/ngx_pagespeed --with-ld-opt='-ljemalloc -qopenmp -parallel' --with-cc-opt='-DTCP_FASTOPEN=23 -xHOST -O3 -ipo -no-prec-div -qopenmp -pthread -unroll-aggressive -qopt-prefetch -parallel -prof-use -prof-dir=/tmp/prof'

./openssl version

OpenSSL 1.0.2a 19 Mar 2015

curl -I https://www.rtv8.nl | grep "Page-Speed"

X-Page-Speed: 1.9.32.3-4480

@marcoroest87

This comment has been minimized.

Copy link

marcoroest87 commented Apr 27, 2015

Hi Hans,

I can't seem the reproduce the issue I'm talking about on the website you mentioned (https://www.rtv8.nl). The javascript on your site loads fine on the first request (js_defer is loaded without issues).

@HansVanEijsden

This comment has been minimized.

Copy link

HansVanEijsden commented Apr 27, 2015

Hi Marco,
Thanks for your effort. It could be a different issue then. I've got the problems in Safari (8.0.5, the newest), but not in Chrome. In Firefox 37.0.2 I have the problem with the slider in top (only showing after reload), but the player on the right is fine.
Something inside me says it could be related, but not exactly the same issue. I'll keep digging. :)

@jeffkaufman

This comment has been minimized.

Copy link
Contributor

jeffkaufman commented Apr 28, 2015

@HansVanEijsden Could you open a separate bug? And include screenshots of what the page should look like vs what it looks like broken?

@jeffkaufman

This comment has been minimized.

Copy link
Contributor

jeffkaufman commented Apr 28, 2015

I've tried buidling Nginx with --with-debug option, but this resulted in
nginx not being able to server any pages at all).

Did this generate any errors in the log? It may be that nginx couldn't serve any pages because a debug-only assertion was failing, and it would be helpful to know what that assertion was.

@jeffkaufman

This comment has been minimized.

Copy link
Contributor

jeffkaufman commented Apr 28, 2015

This is what the access log shows:

94.209.44.170 - - [27/Apr/2015:16:59:36 +0200]
"GET /pagespeed_static/1.JiBnMqyl6S.gif HTTP/1.1" 200 0
"https://demo.hypershop.nl/activity-trackers.html" "Mozilla/5.0
`(Windows NT 6.1; WOW64; rv:37.0) Gecko/20100101 Firefox/37.0"``

Are you saying there was no entry in the access log for js_defer.pbrP1whUgE.js ?

using LoadFromFile instead of using FetchHttps

This shouldn't affect serving js_defer because this is a support file that PageSpeed serves from memory, not from fetching.

@jeffkaufman

This comment has been minimized.

Copy link
Contributor

jeffkaufman commented Apr 28, 2015

What happens if you move:

 include /etc/nginx/conf.d/demo.hypershop.nl/pagespeed.conf;

to before:

location ~* \.(jpe?g|gif|css|svg|png|js|ico|pdf|zip|tar|t?gz|mp3|wav|swf)$ {
@marcoroest87

This comment has been minimized.

Copy link

marcoroest87 commented Apr 28, 2015

Hi Jeff,

First of all thanks for the quick response.

Are you saying there was no entry in the access log for js_defer.pbrP1whUgE.js ?

I've seen that I've accidentially pasted an incorrect bit from my access log earlier.

I've just cleared my access logs and browser history, restarted the webserver and hit a page.
The js_defer was wasn't loaded, and it also wasn't it the accesslog (all other requests for this page were in the access logs).

I Tried move the pagespeed.conf inclusion above this block as you suggested:

What happens if you move:

include /etc/nginx/conf.d/demo.hypershop.nl/pagespeed.conf;

to before:

location ~* \.(jpe?g|gif|css|svg|png|js|ico|pdf|zip|tar|t?gz|mp3|wav|swf)$ {

Unfortunately this showed the exact same result. Even tried commenting this block out all together but still no luck.

I've tried buidling Nginx with --with-debug option, but this resulted in
nginx not being able to server any pages at all).

Did this generate any errors in the log? It may be that nginx couldn't serve any pages because a debug-only assertion was failing, and it would be helpful to know what that assertion was.

I'll rebuild nginx with the debug option later tonight and will post the error log (I believe it was quite large with alot of repeating errors, so I will upload the whole thing.

@marcoroest87

This comment has been minimized.

Copy link

marcoroest87 commented Apr 28, 2015

Here's the error log when starting nginx in debug mode (compiled with same options as above and --with-debug).

The log file was 127MB in about 20 seconds (the same bit repeating over and over so I've added the first 566 lines to a gist:
https://gist.github.com/marcoroest87/cd5b1c61990a133c0b32

@jeffkaufman

This comment has been minimized.

Copy link
Contributor

jeffkaufman commented Apr 29, 2015

Thanks, that log is helpful!

It's failing because it's trying to set up pagespeed and it's not seeing a FileCachePath. Two things to try:

  1. Remove the pagespeed off command from your backend conf

or

  1. Move pagespeed FileCachePath /etc/nginx/pagespeed_cache; to the top level of your config.

These are just workarounds, though, this is a pagespeed configuration bug and we need to fix it.

@marcoroest87

This comment has been minimized.

Copy link

marcoroest87 commented Apr 30, 2015

Hi Jeff,

I'm able to run nginx in debug mode correctly now (adding FileCachePath to nginx.conf helped).
Unfortunately tailing the nginx error log (in debug mode) produces no data while reproducing the issue at hand.

@jeffkaufman

This comment has been minimized.

Copy link
Contributor

jeffkaufman commented May 1, 2015

Ok, good to know. So the FileCachePath issue wasn't related. (We still need to fix that, though.)

@crowell , what do you think?

@jeffkaufman jeffkaufman assigned jeffkaufman and crowell and unassigned jeffkaufman May 1, 2015

@crowell

This comment has been minimized.

Copy link
Contributor

crowell commented May 1, 2015

looking into it now 👍

@crowell

This comment has been minimized.

Copy link
Contributor

crowell commented May 1, 2015

I can reproduce the issue with the DCHECK

(gdb) r
Starting program: /usr/sbin/nginx 
[Thread debugging using libthread_db enabled]
Using host libthread_db library "/lib/x86_64-linux-gnu/libthread_db.so.1".

Program received signal SIGTRAP, Trace/breakpoint trap.
base::debug::(anonymous namespace)::DebugBreak () at third_party/chromium/src/base/debug/debugger_posix.cc:246
246     third_party/chromium/src/base/debug/debugger_posix.cc: No such file or directory.
(gdb) bt
#0  base::debug::(anonymous namespace)::DebugBreak () at third_party/chromium/src/base/debug/debugger_posix.cc:246
#1  0x00000000004d6da5 in base::debug::BreakDebugger () at third_party/chromium/src/base/debug/debugger_posix.cc:259
#2  0x00000000004930b5 in (anonymous namespace)::LogMessageHandler (severity=4, file=<optimized out>, line=<optimized out>, message_start=<optimized out>, str=...) at /home/jeff/ngx_pagespeed-release-1.9.32.3-beta/src/log_mess
age_handler.cc:66
#3  0x00000000004d8270 in logging::LogMessage::~LogMessage (this=0x7fffffffe090, __in_chrg=<optimized out>) at third_party/chromium/src/base/logging.cc:570
#4  0x00000000009c7ff9 in net_instaweb::SystemCachePath::ChildInit (this=0x14ea4b0, cache_clean_worker=0x148f760) at net/instaweb/system/system_cache_path.cc:209
#5  0x000000000061ecdf in net_instaweb::SystemCaches::ChildInit (this=0x1427560) at net/instaweb/system/system_caches.cc:578
#6  0x00000000006289d4 in net_instaweb::SystemRewriteDriverFactory::ChildInit (this=0x1428b60) at net/instaweb/system/system_rewrite_driver_factory.cc:270
#7  0x0000000000498435 in net_instaweb::(anonymous namespace)::ps_init_child_process (cycle=0x1418b30) at /home/jeff/ngx_pagespeed-release-1.9.32.3-beta/src/ngx_pagespeed.cc:3142
#8  0x00000000004310fb in ngx_single_process_cycle (cycle=cycle@entry=0x1418b30) at src/os/unix/ngx_process_cycle.c:298
#9  0x0000000000411147 in main (argc=<optimized out>, argv=<optimized out>) at src/core/nginx.c:416

still digging into it

@crowell

This comment has been minimized.

Copy link
Contributor

crowell commented May 4, 2015

@marcoroest87 your links to the config gists are now 404ing. Could you please reupload them?

@marcoroest87

This comment has been minimized.

@jeffkaufman

This comment has been minimized.

Copy link
Contributor

jeffkaufman commented May 5, 2015

I think the other side of this issue, the one where js_defer doesn't load, is probably related to #962

@marcoroest87

This comment has been minimized.

Copy link

marcoroest87 commented May 6, 2015

@jeffkaufman I'm not sure if this related to #962. I can reproduce this issue in Firefox (Windows) and Safari (OS X).

@oschaaf

This comment has been minimized.

Copy link
Member

oschaaf commented May 6, 2015

@marcoroest87 @HansVanEijsden would either of you be willing to do a joint debugging session to sort this out?

@HansVanEijsden

This comment has been minimized.

Copy link

HansVanEijsden commented May 6, 2015

@oschaaf I would like to help, but these days I'm very busy (work and family things), unfortunately.
I can do short tries and I can also provide server access to you @oschaaf but I'm unable to do extended tests, debugging sessions or Nginx recompiles. I will have more time next month, June.

@marcoroest87

This comment has been minimized.

Copy link

marcoroest87 commented May 7, 2015

@oschaaf Yes I would like to do a joint debugging session to sort this out. I don't have a lot of room to fiddle this in during office hours this week though (CEST timezone). I'm available after office hours though.

@oschaaf

This comment has been minimized.

Copy link
Member

oschaaf commented May 7, 2015

@marcoroest87 Great! I'll send you a PM to initiate.
@HansVanEijsden Thanks! I'll try to figure this out with Marco.

@crowell

This comment has been minimized.

Copy link
Contributor

crowell commented May 8, 2015

@jeffkaufman I've not been able to reproduce this in chrome, just firefox. I don't think that they're (necessarily) related.

@oschaaf

This comment has been minimized.

Copy link
Member

oschaaf commented May 11, 2015

@marcoroest87 and me tried to see what would happen with an ngx_pagespeed trunk-tracking build, but that did not resolve the problem.

@B-ITS-EU

This comment has been minimized.

Copy link

B-ITS-EU commented Jul 18, 2015

Hi, I have the same problem on turnierlive.de and on my shop (test mode - shop.carmodule.de).
What I found out is that the problem seems to disapear in case you disable SPDY.
From the Firebug network monitor it seems that on the first page visit the browser trys to load js_defer through https without SPDY and for some reason i don't know fails with that. On the next visit the browser uses https with SPDY to load the js_defered and this works.
Hope this helps to get the error fixed... For now I have disabled SPDY and the page now seems to load fine all the time - but this is of course no solution.

@martijn0s

This comment has been minimized.

Copy link

martijn0s commented Sep 21, 2015

Hello, we are currently trying to implement pagespeed on our server however we have this exact same issue with nginx 1.9.2 and pagespeed 1.9.32.6-beta. I noticed the problem does not occour on https://demo.hypershop.com/ anymore. @marcoroest87 did you find a solution?

@rfnx

This comment has been minimized.

Copy link
Contributor

rfnx commented Oct 6, 2015

Hello, same problem on my blog. This is really bad because defer_javascript is a very nice filter.
I can easily reproduce the problem on Firefox : Ctrl+F5 to make a full refresh ; but it happens randomly, like 80% of the time.
I can also reproduce the issue on Chrome, but it's even more random.
IE (11 and Edge) never showed the problem (it always works)

@rfnx

This comment has been minimized.

Copy link
Contributor

rfnx commented Oct 6, 2015

I'm sorry but I can't compile the module :( . First, I'm almost sure there is a mistake in the documentation https://github.com/pagespeed/ngx_pagespeed/wiki/Building-PSOL-From-Source.

  • The order of some commands is wrong in the block of code "First build mod_pagespeed at the tag we currently work at:", line 8, 9 and 10 should be 10, 9, 8 (need someone to check that)
  • Also, the line 9 is not clear IMO : git checkout ${BRANCH} ?

But after I fixed these errors, another one appeared during the make :

CC(target) out/Release/obj.target/openssl/third_party/boringssl/src/crypto/bio/socket_helper.o
third_party/boringssl/src/crypto/bio/socket_helper.c: In function ‘bio_ip_and_port_to_socket_and_addr’:
third_party/boringssl/src/crypto/bio/socket_helper.c:42:19: error: storage size of ‘hint’ isn’t known
   struct addrinfo hint, *result, *cur;
                   ^
third_party/boringssl/src/crypto/bio/socket_helper.c:51:9: warning: implicit declaration of function ‘getaddrinfo’ [-Wimplicit-function-declaration]
   ret = getaddrinfo(hostname, port_str, &hint, &result);
         ^
third_party/boringssl/src/crypto/bio/socket_helper.c:54:27: warning: implicit declaration of function ‘gai_strerro ’ [-Wimplicit-function-declaration]
     ERR_add_error_data(2, gai_strerror(ret));
                           ^
third_party/boringssl/src/crypto/bio/socket_helper.c:60:36: error: dereferencing pointer to incomplete type ‘struct addrinfo’
   for (cur = result; cur; cur = cur->ai_next) {
                                    ^
third_party/boringssl/src/crypto/bio/socket_helper.c:79:3: warning: implicit declaration of function ‘freeaddrinfo’ [-Wimplicit-function-declaration]
   freeaddrinfo(result);
   ^
third_party/serf/openssl.target.mk:596: recipe for target 'out/Release/obj.target/openssl/third_party/boringssl/src/crypto/bio/socket_helper.o' failed
make: *** [out/Release/obj.target/openssl/third_party/boringssl/src/crypto/bio/socket_helper.o] Error 1
@crowell

This comment has been minimized.

Copy link
Contributor

crowell commented Oct 6, 2015

the instructions were missing a "git clone", and should be correct now.

This error looks familiar, I think we're currently failing to build openSuSE tumbleweed because of this too.

We have bug here tracking it apache/incubator-pagespeed-mod#1139

@crowell

This comment has been minimized.

Copy link
Contributor

crowell commented Oct 7, 2015

looks like we may be missing some compiler flags -D_XOPEN_SOURCE or -D_POSIX_SOURCE, i need to look over the netdb.h.

@crowell

This comment has been minimized.

Copy link
Contributor

crowell commented Oct 7, 2015

@rfnx pull request apache/incubator-pagespeed-mod#1150 should fix this build error for you. I'll update you when it is merged.

@rfnx

This comment has been minimized.

Copy link
Contributor

rfnx commented Oct 7, 2015

Thanks for the patch, it works and the build ends without error. But the next step in the doc (build psol) is not working : the directory mod_pagespeed/src/net/instaweb/automatic does not exist.

@oschaaf

This comment has been minimized.

Copy link
Member

oschaaf commented Oct 7, 2015

@rfnx That directory has moved to pagespeed/automatic, I updated the docs.

@rfnx

This comment has been minimized.

Copy link
Contributor

rfnx commented Oct 7, 2015

Thanks! But now the -C argument in the make command can be removed (you return in the same directory).
Also, the make does not work at all (with or without -C) because there is a mistake in the Makefile of the directory mod_pagespeed/src/pagespeed/automatic : line 27, it should be "$(shell cd ../..; pwd)" instead of "$(shell cd ../../..; pwd)"

@rfnx

This comment has been minimized.

Copy link
Contributor

rfnx commented Oct 8, 2015

EDIT 2 : Bad news, it still does not work with trunk-tracking + patch, and even with nginx 1.9.5+http2.

But I think the bug happens when pagespeed is rewriting the page. This is why it always happens on first loading when I refresh all caches (browser and pagespeed server cache). The js_defer is maybe not ready yet, and when pagespeed send the page it fails because it is requested at the end of the html code.

@oschaaf

This comment has been minimized.

Copy link
Member

oschaaf commented Oct 8, 2015

@rfnx Thanks for persisting on this. Do you have a url where I could take a look?

@rfnx

This comment has been minimized.

Copy link
Contributor

rfnx commented Oct 8, 2015

You can try on my website https://www.lidocs.me .

@oschaaf

This comment has been minimized.

Copy link
Member

oschaaf commented Oct 8, 2015

@rfnx I had a look at it, and this looks like the same problem as #962 where the stream for the defer javascript file gets cancelled via HTTP2_SESSION_RST_STREAM (but now on http/2 instead of spdy).

@jeffkaufman
In #797 we did something to handle RST_STREAM's in the IPRO flow on spdy. It might be worth double checking what we exactly did to see if we should also do something for the SimpleHandler flow which emits the defer_js.xxxx.js static asset.

@oschaaf

This comment has been minimized.

Copy link
Member

oschaaf commented Oct 9, 2015

@rfnx I tried to reproduce by switching to the bleeding edge and configuring nginx as a forward proxy to your website, but no luck. I can reliably reproduce it on https://www.lidocs.me

To help me replicate your setup in more detail, would it be possible to forward (PM) me these things?:

  • Full (anonymized) nginx configuration
  • The output of nginx -V

It would also be useful to have an error.log trace containing an occurrence of the protocol violation on your system -- it would be best if this could be with nginx compiled in debug mode [1]
The http2 RST frame that nginx sends can only originate from a few places in the nginx code, and the error.log contents might help pointing out the specific code it originates from.

[1] http://nginx.org/en/docs/debugging_log.html

@rfnx

This comment has been minimized.

Copy link
Contributor

rfnx commented Oct 9, 2015

Ok, I'm already compiling nginx with debug enabled !

But how can I send you PM on GitHub ? You mean by email (the address on your github profile ?) ?

@oschaaf

This comment has been minimized.

Copy link
Member

oschaaf commented Oct 9, 2015

@rfnx
Yes, the email address from my github profile would be great, thanks!

@rfnx

This comment has been minimized.

Copy link
Contributor

rfnx commented Oct 9, 2015

@oschaaf The mail is sent ! (check your spam if you don't see it)

@oschaaf

This comment has been minimized.

Copy link
Member

oschaaf commented Oct 9, 2015

ps_simple_handler sends out the headers like this, via send_out_headers_and_body():

  rc = ngx_http_send_header(r);

  if (rc != NGX_OK) {
    return NGX_ERROR;
  }

Other modules, gzip for example, do it like this:

0261     rc = ngx_http_send_header(r);
0262 
0263     if (rc == NGX_ERROR || rc > NGX_OK || r->header_only) {
0264         return rc;
0265     }

Could this be causing the problem with http2/spdy? Looking through ngx_http_spdy_header_filter code, it looks like it wouldn't like us writing out the body when header_only is set, which is what happens now.

So ps_simple_handler should probably change to include the extra checks.
We would then also need to make this flow capable of writing out the body in a next call from nginx.

@rfnx

This comment has been minimized.

Copy link
Contributor

rfnx commented Oct 9, 2015

I'm trying this patch now and the issue seems fixed ! Even on first loading, with clean browser and server cache, the js_defer is loaded correctly and the website works properly :) . Tested on Firefox and Chrome.

EDIT : I answer my own question : yes, it works without the first patch, only the second one seems important (for this issue, but maybe it is still needed to fix another bug ?)

EDIT 2 : There is a new problem now with the last patch : sometimes when I refresh the page, two pagespeed_beacon are sent. But maybe it's not really important ?

@oschaaf

This comment has been minimized.

Copy link
Member

oschaaf commented Oct 10, 2015

@rfnx Great! Thanks for testing. As for the two beacons, I sounds like a potential unrelated issue to this one, I'll take a look at that.
Could you create a pull request with the fix? :-)

@rfnx

This comment has been minimized.

Copy link
Contributor

rfnx commented Oct 10, 2015

Ok I'll try but I'm a noob with github haha.

Should I include the first patch in the pull request ?

@oschaaf

This comment has been minimized.

Copy link
Member

oschaaf commented Oct 10, 2015

@rfnx great. if you need any help let me know!

@rfnx

This comment has been minimized.

Copy link
Contributor

rfnx commented Oct 10, 2015

@oschaaf

This comment has been minimized.

Copy link
Member

oschaaf commented Oct 10, 2015

Re: "Please tell me if I should also include the first patch you sent "
I would prefer to keep the fix minimal for this issue, so no.
I think we want to add the first patch, but later via another separate pull. (because I can't see any good coming from ignoring the return value of ngx_http_output_filter and just returning NGX_OK)

@rfnx

This comment has been minimized.

Copy link
Contributor

rfnx commented Oct 10, 2015

Done ! The first commit referenced here (cd2d1e7) can be deleted, I did a mistake.

jeffkaufman added a commit that referenced this issue Dec 18, 2015

Restore dropped fix for #957
@rfnx fixed #957 in acb89a, but this was accidentally merged to master
instead of trunk-tracking.  I checked for this sort of problem as part
of the 1.10 release, but missed this commit.  Restored.

Fixes #1054

jeffkaufman added a commit that referenced this issue Dec 18, 2015

Restore dropped fix for #957
@rfnx fixed #957 in acb89a, but this was accidentally merged to master
instead of trunk-tracking.  I checked for this sort of problem as part
of the 1.10 release, but missed this commit.  Restored.

Fixes #1054

@pono pono unassigned crowell Jan 8, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment