roadrunner-php / laravel-bridge

πŸŒ‰ RoadRunner ⇆ Laravel bridge πŸ‡ΊπŸ‡¦β€οΈ
https://roadrunner.dev/docs/integration-laravel
MIT License
372 stars 25 forks source link

Disk problem on uploading files. #84

Closed ay4toh5i closed 2 years ago

ay4toh5i commented 2 years ago

First, Thanks for the great package!

I found that lots of temporary files remained in the tmp directory after file uploading. Temporary files will be removed for every request if using nginx + fpm. Is something not working or do I need to hook an event to remove it?

tarampampam commented 2 years ago

Hi @ay4toh5i!

Could you please show your controller code that works with the uploaded files?

ay4toh5i commented 2 years ago

@tarampampam Thank you for the reply!

I was just testing file upload like below. This function reproduces it even if the function is empty.

Route::post('/upload', function (\Illuminate\Http\Request $request) {
    \Log::info(var_export($request->file(), true));
});

My environment

Docker image: php:8.1-alpine3.15 
laravel/framework 8.82.0
spiral/roadrunner-laravel: 5.6.0
tarampampam commented 2 years ago

I think the better strategy for the uploaded files is manual management, like this (warning - pseudo-code):

use Illuminate\Support\Str;
use Illuminate\Http\Request;
use Illuminate\Http\Response;
use Illuminate\Http\JsonResponse;
use Illuminate\Contracts\Filesystem\Filesystem;

class YourController extends \App\Http\Controllers\Controller
{
    /**
     * @param Request    $request
     * @param Filesystem $fs
     *
     * @return Response
     */
    public function handle(Request $request, Filesystem $fs): Response
    {
        $file = $request->file('data');

        if ($file instanceof UploadedFile) {
            $file->move(storage_path('app'), $file_name = Str::random(6) . '_' . $file->getClientOriginalName());

            return new JsonResponse([
                'success'      => true,
                'content_size' => $fs->size($file_name),
            ]);
        }

        return new JsonResponse([
            'success' => false,
            'error'   => 'file was not submitted (use key "data" for file content)',
        ], 400);
    }
}

What do you think about it?

ay4toh5i commented 2 years ago

Does it mean that roadrunner (or roadrunner-laravel) does not clean up temporary files automatically? Do I need to delete the files manually like below when I upload them to S3?

$file = $request->file('data');

Storage::disk('s3')->put('/path/to/file', $file);
unlink($file->getRealPath());

I'm considering writing a cleanup function that hooks AfterRequestHandlingEvent to write the same way in roadrunner and fpm.

tarampampam commented 2 years ago

I have reproduced described behavior and made a listener for fixing this. Please, take a look at PR #87 - could you please test these changes on your side?

tarampampam commented 2 years ago

Ping @ay4toh5i

ay4toh5i commented 2 years ago

@tarampampam Thank you for the fix! I'm going to try it.

ay4toh5i commented 2 years ago

@tarampampam This fix works for me! Thank you so much! It would be nice to have error handling in case the file is moved. I get the error below if the file is moved before the listener cleans up. ErrorException: unlink(/tmp/symfony62046ee674bb59.63713455EdoliF): No such file or directory

Additionally, I noticed that sometimes the error log (error opening the file {"error": "open /tmp/multipart-2768224916: no such file or directory"}) is displayed when the server handles a request after file uploading. Is this a problem roadrunner itself...?

/var/www/html # rr serve
1.644458566951385e+09   debug   rpc     plugin was started      {"address": "tcp://127.0.0.1:6001", "list of the plugins with RPC methods:": ["informer", "resetter"]}
[INFO] RoadRunner server started; version: 2.7.4, buildtime: 2022-01-21T23:03:24+0000
1.6444585683093784e+09  debug   server  worker is allocated     {"pid": 1535, "internal_event_name": "EventWorkerConstruct"}
1.6444585683313837e+09  debug   server  worker is allocated     {"pid": 1539, "internal_event_name": "EventWorkerConstruct"}
1.6444585683333237e+09  debug   http    http server was started {"address": "0.0.0.0:8080"}
1.6444585711260376e+09  info    http    http log        {"status": 200, "method": "GET", "URI": "http://localhost:3030/status", "remote_address": "172.24.0.1", "start": 1644458571.0842135, "elapsed": 0.0418046}
1.6444585870782511e+09  info    server  {"level":"INFO","timestamp":"2022-02-10T02:03:07.077547+00:00","env":"local","message":"/tmp/symfony6204725abb0251.73456105KMLEJK","context":[],"extra":[]}
1.6444585870806518e+09  info    server  {"level":"INFO","timestamp":"2022-02-10T02:03:07.079992+00:00","env":"local","message":"58696459","context":[],"extra":[]}
1.6444585870947886e+09  info    http    http log        {"status": 200, "method": "POST", "URI": "http://localhost:3030/upload", "remote_address": "172.24.0.1", "start": 1644458585.54683, "elapsed": 1.5479301}
1.6444586088064666e+09  error   http    error opening the file  {"error": "open /tmp/multipart-2768224916: no such file or directory"}
1.6444586088189597e+09  info    http    http log        {"status": 200, "method": "GET", "URI": "http://localhost:3030/status", "remote_address": "172.24.0.1", "start": 1644458608.8062859, "elapsed": 0.0126417}
1.6444586094928615e+09  error   http    error opening the file  {"error": "open /tmp/multipart-2768224916: no such file or directory"}
1.6444586095079334e+09  info    http    http log        {"status": 200, "method": "GET", "URI": "http://localhost:3030/status", "remote_address": "172.24.0.1", "start": 1644458609.4927492, "elapsed": 0.0151564}

I was testing like below script.

Route::get('/status', static fn () => 'OK');

Route::post('/upload', function (\Illuminate\Http\Request $request) {
    $file = $request->file('file');
    \Log::info($file->getFileInfo());
    \Log::info($file->getSize());
});
tarampampam commented 2 years ago

Friendly ping @rustatian - maybe can you explain this in more detail?

rustatian commented 2 years ago

Friendly ping @rustatian - maybe can you explain this in more detail?

Hey @tarampampam, sorry, only direct mentioning was turned on (( Multipart files stored in the /tmp (I mean, for Linux πŸ˜ƒ ) directory and RR should clean up them after the request is parsed.

rustatian commented 2 years ago

I'm not sure, why the error opening the file error occurs, If that's possible, could you please file me an issue with the steps on how to reproduce it?

tarampampam commented 2 years ago

Hey dude! But, I can confirm the behavior as described by the topic starter - the temporary files are not cleaned up automatically. Can you check this on the RR side, please?

I have used for the tests tarampampam/laravel-roadrunner-in-docker template with a small modification in the controller:

diff --git a/app/Http/Controllers/TestController.php b/app/Http/Controllers/TestController.php
index 179083d..6c5f053 100644
--- a/app/Http/Controllers/TestController.php
+++ b/app/Http/Controllers/TestController.php
@@ -174,11 +174,11 @@ class TestController extends \Illuminate\Routing\Controller
         $file = $request->file('data');

         if ($file instanceof UploadedFile) {
-            $file->move(storage_path('app'), $file_name = Str::random(6) . '_' . $file->getClientOriginalName());
+//            $file->move(storage_path('app'), $file_name = Str::random(6) . '_' . $file->getClientOriginalName());

             return new JsonResponse([
                 'success'      => true,
-                'content_size' => $fs->size($file_name),
+//                'content_size' => $fs->size($file_name),
                 'duration_sec' => \microtime(true) - $started_at,
                 'memory_bytes' => \memory_get_usage() - $memory_bytes,
             ]);

Just execute in your terminal:

$ git clone ... && cd ...
$ make install
$ make init
$ make up

After that:

$ docker ps | grep web  
35831cef24a4   laravel-roadrunner-in-docker_web     "rr serve -c .rr.loc…"   1 second ago    Up 1 second (health: starting)   0.0.0.0:8080->8080/tcp, :::8080->8080/tcp, 0.0.0.0:8443->8443/tcp, :::8443->8443/tcp   laravel-roadrunner-in-docker_web_1
$ docker exec -ti 358 sh                                             
[user@35831cef24a4] /app # ls -l /tmp/ /app/storage/app/
/app/storage/app/:
total 0
drwxrwxr-x    2 user     user            98 Apr 10  2021 public

/tmp/:
total 0
drwxr-xr-x    3 10001    10001           36 Feb  9 09:55 composer

Both directories are clear. Ok, upload a file (execute in separate terminal, file for test named rss.png in my case):

$ curl -X POST -F "data=@rss.png" "http://127.0.0.1:8080/test/upload"
{"success":true,"duration_sec":0.003936052322387695,"memory_bytes":75376}

And inside the container:

[user@35831cef24a4] /app # ls -l /tmp/ /app/storage/app/
/app/storage/app/:
total 0
drwxrwxr-x    2 user     user            98 Apr 10  2021 public

/tmp/:
total 8
drwxr-xr-x    3 10001    10001           36 Feb  9 09:55 composer
-rw-------    1 user     user          7671 Feb 10 07:33 symfony6204bfdf0c77f4.03487101ehhHKo

As you can see, when the file was not moved by the application - it still exists in the temporary directory. My listener in PR #87 makes an attempt to clean up them:

https://github.com/spiral/roadrunner-laravel/blob/b8c0c91043cea463d155722e812daebf7aeee108/src/Listeners/CleanupUploadedFilesListener.php#L20-L26

But should we do this on the PHP side or RR?

rustatian commented 2 years ago

Sure maaaan πŸ˜„ I will do that a little bit later if you don't mind πŸ’―

rustatian commented 2 years ago

But should we do this on the PHP side or RR?

Good question... I think, that after the multipart request is parsed and loaded in memory, RR should be responsible for cleaning up the temp files. So, this is def an RR issue.

tarampampam commented 2 years ago

@ay4toh5i I have made one change in the listener (clearstatcache was added), and now it looks like:

https://github.com/spiral/roadrunner-laravel/blob/0ca927857991c4a35fd6e502b686129f44c3a68a/src/Listeners/CleanupUploadedFilesListener.php#L20-L30

Could you please test again with this update?

ay4toh5i commented 2 years ago

@tarampampam This works fine for me! Thank you for the quick fix.

tarampampam commented 2 years ago

@ay4toh5i Nice to hear that! I can make a release with this listener so you can use it without any additional code in your application. Would it be useful for you?

ay4toh5i commented 2 years ago

@tarampampam Sure! But, I'm still in the process of testing the introduction of roadrunner, so I'm not in a hurry.

tarampampam commented 2 years ago

πŸ”₯ Release v5.8.0 published! All you need is:

I hope you will be glad to use the RoadRunner πŸ˜„