Skip to content

Asynchronously handle streamed responses #40

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.

Sign up for GitHub

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

Merged
merged 2 commits into from
Jun 27, 2016
Merged

Asynchronously handle streamed responses #40

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
786ae84
Asynchronously handle streamed responses
andig May 27, 2016
6f80ba0
Cleanup and add Content-Length
andig Jun 7, 2016
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
54 changes: 37 additions & 17 deletions Bridges/HttpKernel.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -78,9 +78,9 @@ public function onRequest(ReactRequest $request, HttpResponse $response)

$syRequest = $this->mapRequest($request);

//start buffering the output, so cgi is not sending any http headers
//this is necessary because it would break session handling since
//headers_sent() returns true if any unbuffered output reaches cgi stdout.
// start buffering the output, so cgi is not sending any http headers
// this is necessary because it would break session handling since
// headers_sent() returns true if any unbuffered output reaches cgi stdout.
ob_start();

try {
Expand All @@ -92,9 +92,15 @@ public function onRequest(ReactRequest $request, HttpResponse $response)
} catch (\Exception $exception) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We need a ob_end_clean call also in catch block I think. I guess its better to just let ob_start out of try and add
ob_end_clean after the whole try-catch.

Btw, tabs detected :P

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We need a ob_end_clean call also in catch block I think. I guess its better to just let ob_start out of try and add
ob_end_clean after the whole try-catch.

Btw, tabs detected :P

@marcj Will fix this. Could you also comment on the content-length?

$response->writeHead(500); // internal server error
$response->end();

// end buffering if we need to throw
@ob_end_clean();
throw $exception;
}

// should not receive output from application->handle()
@ob_end_clean();

$this->mapResponse($response, $syResponse);

if ($this->application instanceof TerminableInterface) {
Expand Down Expand Up @@ -170,15 +176,10 @@ protected function mapRequest(ReactRequest $reactRequest)
*/
protected function mapResponse(HttpResponse $reactResponse, SymfonyResponse $syResponse)
{
//end active session
// end active session
if (PHP_SESSION_ACTIVE === session_status()) {
session_write_close();
session_unset(); //reset $_SESSION
}

$content = $syResponse->getContent();
if ($syResponse instanceof SymfonyStreamedResponse) {
$syResponse->sendContent();
session_unset(); // reset $_SESSION
}

$nativeHeaders = [];
Expand All @@ -200,8 +201,8 @@ protected function mapResponse(HttpResponse $reactResponse, SymfonyResponse $syR
}
}

//after reading all headers we need to reset it, so next request
//operates on a clean header.
// after reading all headers we need to reset it, so next request
// operates on a clean header.
header_remove();

$headers = array_merge($nativeHeaders, $syResponse->headers->allPreserveCase());
Expand Down Expand Up @@ -238,14 +239,33 @@ protected function mapResponse(HttpResponse $reactResponse, SymfonyResponse $syR
$headers['Set-Cookie'] = $cookies;
}

$reactResponse->writeHead($syResponse->getStatusCode(), $headers);
if ($syResponse instanceof SymfonyStreamedResponse) {
$reactResponse->writeHead($syResponse->getStatusCode(), $headers);

// asynchronously get content
ob_start(function($buffer) use ($reactResponse) {
$reactResponse->write($buffer);
return '';
}, 4096);

$syResponse->sendContent();

$stdOut = '';
while ($buffer = @ob_get_clean()) {
$stdOut .= $buffer;
// flush remaining content
@ob_end_flush();
$reactResponse->end();
}
else {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no line break please

ob_start();
$content = $syResponse->getContent();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do you expect something to happen by calling this getter? Or why are you wrapping it with ob_start?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@marcj I don't but this is actually already the case in the current implementation with buffering started here https://github.com/php-pm/php-pm-httpkernel/blob/master/Bridges/HttpKernel.php#L84

Is there a specific function you had intended to buffer in the current implementation? Then I could limit it to that one.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok, then let it that way 👍

@ob_end_flush();

$reactResponse->end($stdOut . $content);
if (!isset($headers['Content-Length'])) {
$headers['Content-Length'] = strlen($content);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What if we have utf8 content? Regarding http spec, content-length is not mandatory and the client reads until connection drops. I'd go the safe path and don't set content-length for now. Did you experience issues with not setting it?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What if we have utf8 content?

Imho content-length is the number of octets. strlen will do that correctly for utf8, too

Did you experience issues with not setting it?

It excludes "simple" clients since react will deliver a chunked response if content-length is not present. So the argument goes more in direction of react than this workaround? I'm inventing the content-length to prevent react from inventing chunked...

Ok?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ah ok! :)

}

$reactResponse->writeHead($syResponse->getStatusCode(), $headers);
$reactResponse->end($content);
}
}

/**
Expand Down