Slow transfers block entire server

[thekid]

Solution is to use -m prefork, but can we somehow insert up- and downloads to the socket loop (using yield maybe)?

---

Update: Using yield worked as expected, see #70 (comment)

[thekid]

Asynchronous output I/O

Here's some code from the xp-forge/frontend library:

$res->header('Content-Type', 'text/html; charset=utf-8');
$res->header('X-Content-Type-Options', 'nosniff');

$out= $res->stream();
try {
  $this->templates->write($this->template, $this->context, $out);
} finally {
  $out->close();
}

The easiest way would be to simply attach a closure as follows:

$res->header('Content-Type', 'text/html; charset=utf-8');
$res->header('X-Content-Type-Options', 'nosniff');

$res->attach(function() {
  $out= $res->stream();
  try {
    $this->templates->write($this->template, $this->context, $out); // template transformation writes to $out
  } finally {
    $out->close();
  }
});

Here's another usecase from FilesFrom:

$response->answer(200, 'OK');
$response->transfer($file->in(), $mimeType, $file->size());

That could even be left unchanged, as transfer() would simply keep the file input stream open. What wouldn't work is to delete the file after transferring it (e.g. because we're using a temporary file), but we could rewrite that to something promise-style:

$response->answer(200, 'OK');
$response->transfer($temp->in(), $mimeType, $temp->size())->then(fn() => $temp->unlink());

Proof of concept

Here's a patch to implement this for the transfer() and send() methods:

diff --git a/src/main/php/web/Response.class.php b/src/main/php/web/Response.class.php
index d23776d..408762b 100755
--- a/src/main/php/web/Response.class.php
+++ b/src/main/php/web/Response.class.php
@@ -176,16 +176,12 @@ class Response {
    */
   public function transfer($in, $mediaType= 'application/octet-stream', $size= null) {
     $this->headers['Content-Type']= [$mediaType];
-
-    $out= $this->stream($size);
-    try {
+    $this->async= [$size, function() use($in) {
       while ($in->available()) {
-        $out->write($in->read());
+        yield $in->read();
       }
-    } finally {
-      $out->close();
       $in->close();
-    }
+    }];
   }
 
   /**
@@ -196,12 +192,8 @@ class Response {
    */
   public function send($content, $mediaType= 'text/html') {
     $this->headers['Content-Type']= [$mediaType];
-
-    $out= $this->stream(strlen($content));
-    try {
-      $out->write($content);
-    } finally {
-      $out->close();
-    }
+    $this->async= [strlen($content), function() use($content) {
+      yield $content;
+    }];
   }
 }
\ No newline at end of file
diff --git a/src/main/php/xp/web/srv/HttpProtocol.class.php b/src/main/php/xp/web/srv/HttpProtocol.class.php
index 3539ba0..9c0855d 100755
--- a/src/main/php/xp/web/srv/HttpProtocol.class.php
+++ b/src/main/php/xp/web/srv/HttpProtocol.class.php
@@ -115,6 +115,15 @@ class HttpProtocol implements ServerProtocol {
       try {
         $this->application->service($request, $response);
         $this->logging->log($request, $response);
+
+        if (isset($response->async)) {
+          $out= $response->stream($response->async[0]);
+          foreach ($response->async[1]() as $chunk) {
+            $out->write($chunk);
+          }
+          $out->close();
+        }
+
       } catch (Error $e) {
         $this->sendError($request, $response, $e);
       } catch (\Throwable $e) {
diff --git a/src/test/php/web/unittest/ResponseTest.class.php b/src/test/php/web/unittest/ResponseTest.class.php
index a78baf4..0e6ba8c 100755
--- a/src/test/php/web/unittest/ResponseTest.class.php
+++ b/src/test/php/web/unittest/ResponseTest.class.php
@@ -16,6 +16,14 @@ class ResponseTest extends \unittest\TestCase {
    * @throws unittest.AssertionFailedError
    */
   private function assertResponse($expected, $response) {
+    if (isset($response->async)) {
+      $out= $response->stream($response->async[0]);
+      foreach ($response->async[1]() as $chunk) {
+        $out->write($chunk);
+      }
+      $out->close();
+    }
+
     $this->assertEquals($expected, $response->output()->bytes());
   }
 
diff --git a/src/test/php/web/unittest/handler/FilesFromTest.class.php b/src/test/php/web/unittest/handler/FilesFromTest.class.php
index 9590db0..1c7a601 100755
--- a/src/test/php/web/unittest/handler/FilesFromTest.class.php
+++ b/src/test/php/web/unittest/handler/FilesFromTest.class.php
@@ -49,6 +49,14 @@ class FilesFromTest extends \unittest\TestCase {
    * @throws unittest.AssertionFailedError
    */
   private function assertResponse($expected, $response) {
+    if (isset($response->async)) {
+      $out= $response->stream($response->async[0]);
+      foreach ($response->async[1]() as $chunk) {
+        $out->write($chunk);
+      }
+      $out->close();
+    }
+
     $this->assertEquals($expected, preg_replace(
       '/[a-z]{3}, [0-9]{2} [a-z]{3} [0-9]{4} [0-9:]{8} GMT/i',
       '<Date>',

This would also have the potential benefit of simplifying the whole flush() / flushed handling.

[thekid]

Ideas for API:

// Argument may be one of the following
$arg= 'Bytes';
$arg= $file->in();
$arg= function() { yield 'Bytes'; });

// If length == null, use Transfer-Encoding: chunked, add Content-Length header otherwise
$response->write($length, $arg);

Ideas for wrapping:

// Convert everything to an input stream
if ($arg instanceof InputStream) {
  $this->transfer= $arg;
} else if ($arg instanceof \Generator) {
  $this->transfer= new Yielding($arg);
} else {
  $this->transfer= new MemoryInputStream($arg);
}

// Convert everything to a generator
if ($arg instanceof InputStream) {
  $this->transfer= function() use($in) { while ($in->available()) yield $in->read(); $in->close(); }
} else if ($arg instanceof \Generator) {
  $this->transfer= $arg;
} else {
  $this->transfer= function() use($arg) { yield (string)$arg; };
}

[thekid]

Yield

A completely different approach would be to make the handlers interruptible. The benefit of this approach would be that synchronous handlers would still work without change, while more sophisticated implementations like the built-in static file handler could provide interruptible behavior.

// Download usecase
return ['/' => function($req, $res) {
  $file= new File('download.mp4');
  $res->header('Content-Type', 'video/mp4');

  yield;     // <-- This line would defer transfer
  $in= $file->in();
  $out= $res->stream($file->size());
  while ($in->available()) {
    $out->write($in->read());
    yield;   // <-- This line would yield control after a write
  }
}];

// Upload usecase
return ['/' => function($req, $res) {
  $target= new Folder(...);
  foreach ($req->multipart()->files() as $name => $file) {
    $file->transfer($target);
    yield;   // <-- This line would yield control after every file
  }

  yield; // <-- This line would be necessary to separate input from output
  $res->answer(201);
}];

Most probably the line to separate input from output would need to return some kind of special value to make it distinguishable from the oder yield statements. Maybe yield $res->answer(201);?

Proof of concept

This first step would make handlers with yield inside work seamlessly:

diff --git a/src/main/php/web/Application.class.php b/src/main/php/web/Application.class.php
index 3b0c2b0..fe46869 100755
--- a/src/main/php/web/Application.class.php
+++ b/src/main/php/web/Application.class.php
@@ -58,10 +58,10 @@ abstract class Application implements \lang\Value {
    *
    * @param  web.Request $request
    * @param  web.Response $response
-   * @return void
+   * @return var
    */
   public function service($request, $response) {
-    $this->routing()->service($request, $response);
+    return $this->routing()->service($request, $response);
   }
 
   /** @return string */
diff --git a/src/main/php/xp/web/srv/HttpProtocol.class.php b/src/main/php/xp/web/srv/HttpProtocol.class.php
index 3539ba0..c286e54 100755
--- a/src/main/php/xp/web/srv/HttpProtocol.class.php
+++ b/src/main/php/xp/web/srv/HttpProtocol.class.php
@@ -113,7 +113,13 @@ class HttpProtocol implements ServerProtocol {
       }
 
       try {
-        $this->application->service($request, $response);
+        $r= $this->application->service($request, $response);
+        if ($r instanceof \Generator) {
+          while ($r->valid()) {
+            $r->next();
+          }
+        }
+
         $this->logging->log($request, $response);
       } catch (Error $e) {
         $this->sendError($request, $response, $e);

The next step would be to embed this in a server implementation that supports this.

[thekid]

Here's a server implementation including demo to support both handlers with yield and those that simply return immediately:

https://gist.github.com/thekid/1ac203dd383ac159313e04acc39f1f5a

The following code makes this file upload interruptible, meaning the server will be able to serve requests while streaming files:

$ diff -u Upload.class.php Upload.class.php-async.php
--- Upload.class.php    2021-03-29 18:08:23.921567300 +0200
+++ Upload.class.php-async.php  2021-03-29 18:08:40.404954600 +0200
@@ -45,7 +45,12 @@
             Console::write('    ', $name, ' => ', $part);
             if (Part::FILE === $part->kind()) {
               try {
-                $streamed= strlen(Streams::readAll($part));
+                $streamed= 0;
+                while ($part->available()) {
+                  $streamed+= strlen($part->read());
+                  yield;
+                }
+
                 $bytes+= $streamed;
                 $files++;
                 Console::writeLine(' -> ', $streamed);

As you can see, replacing the blocking Streams::readAll() call by a while loop with yield inside is the only thing necessary.

[thekid]

Here's a server implementation

Now exists as PR: xp-framework/networking#17

[thekid]

This also means code like this has to be rewritten:

$target= new File(...);
$res->transfer($target->in(), 'video/mp4', $target->size());

...to the following:

$target= new File(...);
$res->header('Content-Type', 'video/mp4');

try {
  $out= $res->stream($target->size());
  do {
    $out->write($target->read());
    yield;
  } while (!$target->eof());
} finally {
  $target->close();
}

Maybe we could have an async version of transfer(), which could then be called like this?

$target= new File(...);
yield from $res->transmit($target->in(), 'video/mp4', $target->size());

There's a risk here, however: If we forget the yield from, nothing happens.

[thekid]

Async server has now been released in https://github.com/xp-framework/networking/releases/tag/v10.1.0

[thekid]

Released in https://github.com/xp-forge/web/releases/tag/v2.8.0