Skip to content

Support http connection keep-alive #9

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

Closed
barrylb wants to merge 4 commits into from
Closed

Support http connection keep-alive #9

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
f14fec7
Support http connection keep-alive
barrylb Jul 29, 2014
f468514
fix indent
barrylb Aug 14, 2014
6e2087f
fix indent
barrylb Aug 14, 2014
09cfa23
Removal of listeners from RequestHeaderParser
barrylb Sep 23, 2014
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
3 changes: 1 addition & 2 deletions src/RequestHeaderParser.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,9 +26,8 @@ public function feed($data)

if (false !== strpos($this->buffer, "\r\n\r\n")) {
list($request, $bodyBuffer) = $this->parseRequest($this->buffer);

$this->buffer = '';
$this->emit('headers', array($request, $bodyBuffer));
$this->removeAllListeners();
}
}

Expand Down
9 changes: 6 additions & 3 deletions src/Response.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -107,7 +107,7 @@ public function write($data)
return $flushed;
}

public function end($data = null)
public function end($data = null, $keepAlive = false)

@clue clue Apr 12, 2015

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Can you give some example code on how you expect this to be used?

It's my understanding that the Response object should behave like a WritableStreamInterface, i.e. end()ing this only terminates its "virtual stream" and should not have any effect on the underlying socket connection. IMO it should be up to the server to determine when the close the underlying connection.

Any info on how other HTTP libs handle this?

@barrylb barrylb Apr 14, 2015

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Yeah it could be neater for the http framework to handle closing connections rather than leaving each request handler do it.

The Request object could process the received headers according to http://tools.ietf.org/html/rfc7230#section-6.3 to know when to close a connection. In addition the writeHead function would need to check for presence of a "Connection: Close" header in the response, to close the connection when present.

This would seem to mimic the behaviour of PHP scripts running under Apache.

{
if (null !== $data) {
$this->write($data);
Expand All @@ -117,9 +117,12 @@ public function end($data = null)
$this->conn->write("0\r\n\r\n");
}

$this->emit('end');
$this->emit('end', array($keepAlive));
$this->removeAllListeners();
$this->conn->end();

if (!$keepAlive) {
$this->conn->end();
}
}

public function close()
Expand Down
23 changes: 19 additions & 4 deletions src/Server.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,6 @@ public function __construct(SocketServerInterface $io)
$this->io = $io;

$this->io->on('connection', function ($conn) {
// TODO: http 1.1 keep-alive
// TODO: chunked transfer encoding (also for outgoing data)
// TODO: multipart parsing

Expand All @@ -25,8 +24,6 @@ public function __construct(SocketServerInterface $io)
// attach remote ip to the request as metadata
$request->remoteAddress = $conn->getRemoteAddress();

$this->handleRequest($conn, $request, $bodyBuffer);

$conn->removeListener('data', array($parser, 'feed'));
$conn->on('end', function () use ($request) {
$request->emit('end');
Expand All @@ -40,13 +37,18 @@ public function __construct(SocketServerInterface $io)
$request->on('resume', function () use ($conn) {
$conn->emit('resume');
});

$this->handleRequest($conn, $parser, $request, $bodyBuffer);
});

$conn->on('data', array($parser, 'feed'));
$conn->on('end', function () use ($parser) {
$parser->removeAllListeners();
});
});
}

public function handleRequest(ConnectionInterface $conn, Request $request, $bodyBuffer)
public function handleRequest(ConnectionInterface $conn, RequestHeaderParser $parser, Request $request, $bodyBuffer)
{
$response = new Response($conn);
$response->on('close', array($request, 'close'));
Expand All @@ -57,6 +59,19 @@ public function handleRequest(ConnectionInterface $conn, Request $request, $body
return;
}

$response->on('end', function ($keepAlive) use ($conn, $parser) {
$conn->removeAllListeners(); // stop data being sent to this Request instance
if ($keepAlive) {
$conn->on('data', array($parser, 'feed')); // resume sending data to RequestHeaderParser
$conn->on('end', function () use ($parser) {
$parser->removeAllListeners();
});
}
else {
$parser->removeAllListeners();
}
});

@cboden cboden Aug 14, 2014

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I believe we should also do a $parser->removeAllListeners() call here.

@barrylb barrylb Aug 14, 2014

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

When keeping the connection alive, the RequestHeaderParser continues to emit 'header' events so removing the listeners would prevent subsequent header events from being handled.

@cboden cboden Aug 14, 2014

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Ah, good point! I was/am worried about resource cleanup that is no longer happening here.


$this->emit('request', array($request, $response));
$request->emit('data', array($bodyBuffer));
}
Expand Down