Skip to content

Commit

Permalink
Set Connection: close when downloading images for twitter
Browse files Browse the repository at this point in the history
  • Loading branch information
DaveRandom committed Apr 18, 2017
1 parent 5148cef commit d0ade44
Showing 1 changed file with 10 additions and 3 deletions.
13 changes: 10 additions & 3 deletions src/Plugins/Tweet.php
Expand Up @@ -3,7 +3,8 @@
namespace Room11\Jeeves\Plugins;

use Amp\Artax\HttpClient;
use Amp\Artax\Response;
use Amp\Artax\Request as HttpRequest;
use Amp\Artax\Response as HttpResponse;
use PeeHaa\AsyncTwitter\Api\Client\Client as TwitterClient;
use PeeHaa\AsyncTwitter\Api\Client\ClientFactory as TwitterClientFactory;
use PeeHaa\AsyncTwitter\Api\Client\Exception\RequestFailed as TwitterRequestFailedException;
Expand Down Expand Up @@ -128,8 +129,14 @@ private function replaceImages(\DOMDocument $dom)
$target = 'https:' . $target;
}

/** @var Response $response */
$response = yield $this->httpClient->request($target);
/** @var HttpResponse $response */
$request = (new HttpRequest)
->setMethod('GET')
->setUri($target)
->setHeader('Connection', 'close');

This comment has been minimized.

Copy link
@staabm

staabm Apr 18, 2017

Contributor

why do we need this header especially for twitter? should we use it in more places in this codebase?

This comment has been minimized.

Copy link
@PeeHaa

PeeHaa Apr 18, 2017

Member

We also already needed it for PHP's wiki pages (the RFCs retriever / parser).

This comment has been minimized.

Copy link
@staabm

staabm Apr 18, 2017

Contributor

what I meant is: should we have a common method to emit a http-request so we dont miss it in other places? is the need common enough to have it in all http requests?

This comment has been minimized.

Copy link
@DaveRandom

DaveRandom Apr 18, 2017

Author Member

This is a bug in artax, really /cc @bwoebi @kelunik @rdlowrey @trowski. It shouldn't be necessary to do this manually, the client should handle this correctly - unless the server is misbehaving (I haven't actually inspected the request/response, I just did this on a hunch).

We don't want to do this by default, it's a work-around only when strictly necessary, ideally the HTTP client layer should be allowed to manage it's own persistent connections - mostly it works.

I will generate some request/response traces without this patch for inspection so we can try and work out if this is a bug in artax or a misbehaving server - since we are talking about imgur and twitter I suspect it's the former...

This comment has been minimized.

Copy link
@PeeHaa

PeeHaa Apr 18, 2017

Member

Can you create a ticket over there @DaveRandom ?

This comment has been minimized.

Copy link
@DaveRandom

DaveRandom Apr 18, 2017

Author Member

I will when I have something concrete to report, need to verify that the server response actually contained some mechanism for detecting the end first.

There are at least 4 ways of doing it that I can think of, there may be others I can't think of, will figure out the nature of the problem before creating a report.

This comment has been minimized.

Copy link
@PeeHaa

PeeHaa Apr 18, 2017

Member

❤️


/** @var HttpResponse $response */
$response = yield $this->httpClient->request($request);
$tmpFilePath = \Room11\Jeeves\DATA_BASE_DIR . '/' . uniqid('twitter-media-', true);
yield \Amp\File\put($tmpFilePath, $response->getBody());

Expand Down

0 comments on commit d0ade44

Please sign in to comment.