_HTTPConnection: check location on _should_follow_redirect() and retain safe request when following redirects

[garenchan]

1. http://www.w3.org/Protocols/rfc2616/rfc2616-sec10.html#sec10.3.3
   According to the spec, the temporary URI may not be specified when the request method was HEAD.
   while check _should_follow_redirect(), doesn't verify whether location is specified.
   

   tornado/tornado/simple_httpclient.py

   Lines 493 to 496 in 9bb6463

   	def _should_follow_redirect(self):
   	return (self.request.follow_redirects and
   	self.request.max_redirects > 0 and
   	self.code in (301, 302, 303, 307, 308))

2. AsyncHTTPClient always use GET method on following redirects：
   

   tornado/tornado/simple_httpclient.py

   Lines 517 to 518 in 9bb6463

   	if self.code in (302, 303):
   	new_request.method = "GET"

   but while use CURL with HEAD method, CURL will also use HEAD method on following redirects:

# curl -v -L --head http://172.18.231.164:9999/test
* Hostname was NOT found in DNS cache
*   Trying 172.18.231.164...
* Connected to 172.18.231.164 (172.18.231.164) port 9999 (#0)
> HEAD /test HTTP/1.1
> User-Agent: curl/7.35.0
> Host: 172.18.231.164:9999
> Accept: */*
> 
< HTTP/1.1 302 Found
HTTP/1.1 302 Found
< Location: /dest
Location: /dest
* Server TornadoServer/5.0.2 is not blacklisted
< Server: TornadoServer/5.0.2
Server: TornadoServer/5.0.2
< Content-Length: 0
Content-Length: 0
< Content-Type: text/html; charset=UTF-8
Content-Type: text/html; charset=UTF-8
< Date: Fri, 01 Jun 2018 11:21:35 GMT
Date: Fri, 01 Jun 2018 11:21:35 GMT

< 
* Connection #0 to host 172.18.231.164 left intact
* Issue another request to this URL: 'http://172.18.231.164:9999/dest'
* Found bundle for host 172.18.231.164: 0xba18f0
* Re-using existing connection! (#0) with host 172.18.231.164
* Connected to 172.18.231.164 (172.18.231.164) port 9999 (#0)
> HEAD /dest HTTP/1.1
> User-Agent: curl/7.35.0
> Host: 172.18.231.164:9999
> Accept: */*
> 
< HTTP/1.1 200 OK
HTTP/1.1 200 OK
< Content-Length: 10
Content-Length: 10
* Server TornadoServer/5.0.2 is not blacklisted
< Server: TornadoServer/5.0.2
Server: TornadoServer/5.0.2
< Etag: "9c0e5ce7f4b8696ea9ab7095b94bb717d09718a9"
Etag: "9c0e5ce7f4b8696ea9ab7095b94bb717d09718a9"
< Content-Type: text/html; charset=UTF-8
Content-Type: text/html; charset=UTF-8
< Date: Fri, 01 Jun 2018 11:21:35 GMT
Date: Fri, 01 Jun 2018 11:21:35 GMT

< 
* Connection #0 to host 172.18.231.164 left intact

[bdarnell]

I think you're misreading the spec. The Location header SHOULD be given no matter what the request method was. The only difference with HEAD is whether there should be a body for clients that don't follow redirects automatically (there's nothing special about HEAD with 302 in this respect; that's how HEAD always works).

Also, RFC2616 is obsolete. The current definition of the 302 response code is https://tools.ietf.org/html/rfc7231#section-6.4.3.

[bdarnell]

The actual code change is fine, but I think I'd add it in to the same return expression above (and self.headers.get('Location') is not None) instead of making this method have two separate return statements.

[garenchan]

Sorry, I really misunderstood the spec because of my poor English！
When following 301/302/303 redirects, should we retain the original request method if it is GET, HEAD or OPTIONS?

[ploxiln]

minor typo: N should be M in "NETHODS"

[garenchan]

Thanks for your reminding. I‘m too careless.

[bdarnell]

Thanks!