Skip to content
This repository has been archived by the owner on Nov 15, 2023. It is now read-only.

Disk problem on uploading files. #84

Closed
ay4toh5i opened this issue Feb 5, 2022 · 19 comments · Fixed by #87
Closed

Disk problem on uploading files. #84

ay4toh5i opened this issue Feb 5, 2022 · 19 comments · Fixed by #87
Assignees
Labels
type:question Further information is requested

Comments

@ay4toh5i
Copy link

ay4toh5i commented Feb 5, 2022

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?

@ay4toh5i ay4toh5i added the type:question Further information is requested label Feb 5, 2022
@tarampampam
Copy link
Collaborator

Hi @ay4toh5i!

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

@ay4toh5i
Copy link
Author

ay4toh5i commented Feb 5, 2022

@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
Copy link
Collaborator

tarampampam commented Feb 5, 2022

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
Copy link
Author

ay4toh5i commented Feb 6, 2022

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 tarampampam mentioned this issue Feb 9, 2022
5 tasks
@tarampampam
Copy link
Collaborator

tarampampam commented Feb 9, 2022

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
Copy link
Collaborator

Ping @ay4toh5i

@ay4toh5i
Copy link
Author

ay4toh5i commented Feb 9, 2022

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

@ay4toh5i
Copy link
Author

ay4toh5i commented Feb 10, 2022

@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
Copy link
Collaborator

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

@rustatian
Copy link

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
Copy link

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
Copy link
Collaborator

tarampampam commented Feb 10, 2022

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 "[email protected]" "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
Copy link

Sure maaaan 😄 I will do that a little bit later if you don't mind 💯

@rustatian
Copy link

rustatian commented Feb 10, 2022

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
Copy link
Collaborator

tarampampam commented Feb 10, 2022

@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
Copy link
Author

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

@tarampampam
Copy link
Collaborator

@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
Copy link
Author

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

@tarampampam
Copy link
Collaborator

🔥 Release v5.8.0 published! All you need is:

  • Update the package using composer
  • Add in your config (config/roadrunner.php) the following line (without comment, of course)

I hope you will be glad to use the RoadRunner 😄

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
type:question Further information is requested
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants