Skip to content
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

Connection: keep-alive #22

Open
wants to merge 14 commits into
base: master
Choose a base branch
from
Open

Connection: keep-alive #22

wants to merge 14 commits into from

Conversation

thekid
Copy link
Member

@thekid thekid commented Oct 3, 2017

Improves performance of peer.http.SocketHttpTransport by implementing keep-alive semantics.

Performance

Same script as in #21, 100 queries using the Neo4J REST API:

Master Master with keep-alive This branch With keep-alive
0.561 seconds 🔴 (hangs) 0.518 seconds 0.185 seconds

Usage

Users need to pass the keep-alive header explicitely, for example as follows:

index 8aff954..bc132fa 100755
--- a/src/main/php/com/neo4j/HttpProtocol.class.php
+++ b/src/main/php/com/neo4j/HttpProtocol.class.php
@@ -38,6 +38,7 @@ class HttpProtocol extends Protocol {
     $req->setMethod('POST');
     $req->setTarget($this->base.'/transaction/commit');
     $req->setHeader('X-Stream', 'true');
+    $req->setHeader('Connection', 'keep-alive');
     $req->setHeader('Content-Type', 'application/json');
     $req->setParameters(new RequestData(Json::of($payload, $this->json)));

. ReadLength for responses with Content-Length header
. ReadChunks for response with Content-Transfer-Encoding: chunked
. HttpInputStream will handle the "otherwise" case
@thekid thekid mentioned this pull request Oct 3, 2017
@thekid
Copy link
Member Author

thekid commented Oct 4, 2017

Saw this today in a header enumeration:

"Keep-Alive": "timeout=5, max=99"

https://tools.ietf.org/id/draft-thomson-hybi-http-timeout-01.html#rfc.section.2
https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Keep-Alive

Non-standard, but seems to have widespread support; we should consider this in the future.

public function connect($connectTimeout, $readTimeout) {
if (false === $this->reuseable) {
$this->socket->close();
} else if ($this->socket->isConnected()) {
Copy link
Member Author

Choose a reason for hiding this comment

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

Is this check enough? If the server closes the connection (e.g. because of a timeout on a reused connection), we would probably receive an EOF here, but isConnected() would still be true,

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, this actually is a problem; and it seems eof() could help here. In need of some more real-life testing!


use peer\SocketInputStream;

class Channel implements \io\streams\InputStream {
Copy link
Member Author

Choose a reason for hiding this comment

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

This class needs some tests!

Copy link
Member Author

Choose a reason for hiding this comment

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

Script used for integration testing timeouts and reconnection behaviors:

<?php namespace keep_alive;

use peer\http\HttpConnection;
use util\cmd\Console;
use io\streams\Streams;
use lang\Throwable;

$c= new HttpConnection($argv[1]);

for ($i= 0; $i < 10; $i++) {
  try {
    $r= $c->get([], ['Connection' => 'keep-alive']);
    Console::writeLine(date('r'), ': ', $r);
    Streams::readAll($r->in());
  } catch (Throwable $t) {
    Console::writeLine(date('r'), ': ', $t);
  }

  $delay= rand(100, 6000);
  Console::writeLinef('ZZ %.3f seconds', $delay / 1000);
  usleep($delay * 1000);
}

@thekid
Copy link
Member Author

thekid commented Oct 7, 2017

I think this is ready to get some real-life exposure; this could go into applications as:

"require" : {
  "xp-framework/http": "dev-feature/keep-alive as 9.1.0"
}

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

Successfully merging this pull request may close these issues.

1 participant