fix: handle client disconnect properly with ignore_user_abort true #207

kesselb · 2023-05-24T14:07:30Z

#205 but with tests ;)

ignore_user_abort(true) instruct php to not abort the script on client disconnect. To avoid reading the entire stream and dismissing the data afterward, check between the chunks if the client is still there.

codecov · 2023-05-25T13:42:45Z

Codecov Report

Merging #207 (f2a1635) into master (c2cd0a5) will decrease coverage by 0.11%.
The diff coverage is 50.00%.

❗ Current head f2a1635 differs from pull request most recent head c574923. Consider uploading reports for the commit c574923 to get more accurate results

@@             Coverage Diff              @@
##             master     #207      +/-   ##
============================================
- Coverage     94.50%   94.40%   -0.11%     
- Complexity      256      258       +2     
============================================
  Files            15       15              
  Lines           873      875       +2     
============================================
+ Hits            825      826       +1     
- Misses           48       49       +1

Impacted Files	Coverage Δ
lib/Sapi.php	`98.13% <50.00%> (-0.92%)`	⬇️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

yevon · 2023-06-10T18:24:26Z

Hi, any news on this one? Thanks!

FedericoHeichou · 2023-06-20T13:55:11Z

Why hasn't it been merged yet? This commit fixed all my problems with streaming and downloads

szaimen · 2023-06-20T13:58:50Z

@staabm could you please have a look at the PR? Thanks a lot! :)

staabm

looks great thanks - verified the test locally.

staabm · 2023-06-21T20:11:59Z

@phil-davis do you have an idea on the cs error on php7.4 only?

ignore_user_abort(true) instruct php to not abort the script on client disconnect. To avoid reading the entire stream and dismissing the data afterward, check between the chunks if the client is still there. Signed-off-by: Daniel Kesselberg <mail@danielkesselberg.de>

phil-davis · 2023-06-26T09:01:54Z

I fixed CI with PR #208 and then rebased here.

phil-davis · 2023-06-26T09:06:32Z

And pushed a small change to get phpstan passing.

phil-davis · 2023-06-26T09:22:17Z

lib/Sapi.php

+                    // Abort on client disconnect.
+                    // With ignore_user_abort(true), the script is not aborted on client disconnect.
+                    // To avoid reading the entire stream and dismissing the data afterward, check between the chunks if the client is still there.
+                    if (1 === ignore_user_abort() && 1 === connection_aborted()) {


https://www.php.net/manual/en/function.ignore-user-abort.php
This PHP function does return an int - looking at its name, I would have assumed it returned a bool !
So this code does look OK.

I had the same experience ;)

just figured, I should cover this better in PHPStan: phpstan/phpstan-src#2489

also connection_aborted impurity will be covered by phpstan/phpstan-src#2555

staabm · 2023-06-26T09:33:32Z

And pushed a small change to get phpstan passing.

awesome, thanks phil.

phil-davis · 2023-06-26T09:49:35Z

I will sort out backporting this (and any other recent fixes) to the 6.x and 5.x release series.

sabre/dav currently only uses sabre/http v5, and it would be nice to have the fixes picked up there by a patch release(s) here in sabre/http

phil-davis · 2023-06-26T11:05:51Z

This fix is now available in 5.1.7 6.0.1 and 7.0.1

yevon · 2023-07-21T19:42:40Z

This should be reopened, 27.0.1 did not fix the issue. Nextcloud keeps downloading full files even after canceling the file download. Just try to download a 100gb file and cancelling it immediately. You can execute this command to check it out:

sudo iotop -o -a

FedericoHeichou · 2023-07-21T19:54:43Z

This should be reopened, 27.0.1 did not fix the issue. Nextcloud keeps downloading full files even after canceling the file download. Just try to download a 100gb file and cancelling it immediately. You can execute this command to check it out:

sudo iotop -o -a

I bet you are not using the patched version.
Check with composer and update the library https://github.com/nextcloud/3rdparty/tree/v27.0.1

yevon · 2023-07-21T20:08:45Z

Sorry I jumped here from a linked nextcloud issue instead of nextcloud repo! haha, they have not fixed the issue, not sabre!

FedericoHeichou · 2023-07-21T20:28:30Z

Sorry I jumped here from a linked nextcloud issue instead of nextcloud repo! haha, they have not fixed the issue, not sabre!

Np, I am the one who linked the PR to that issue.
In this moment I can't try to reproduce the issue, maybe I'll try the next week, but the parts using sabre in the correct way should work.
I suggest to add more informations in your comment in the nextcloud issue, else I don't know if someone can reproduce it

szaimen · 2023-07-21T21:26:37Z

The issue is fixed with Nextcloud 28 which is not released yet.

Ruakij · 2023-07-22T10:03:37Z

lib/Sapi.php

+                    // Abort on client disconnect.
+                    // With ignore_user_abort(true), the script is not aborted on client disconnect.
+                    // To avoid reading the entire stream and dismissing the data afterward, check between the chunks if the client is still there.
+                    if (1 === ignore_user_abort() && 1 === connection_aborted()) {


I'd assume 0 means false and 1 is true?
If so, this code falsely breaks when the connection was aborted AND we want to ignore that?

Or did i mix something up?

https://www.php.net/manual/en/function.ignore-user-abort.php

1 === ignore_user_abort() means "ignore user abort has been enabled" (somewhere in "the system" or "the code" prior to this point)

1 === connection_aborted() means that a connection has "gone away".

When these 2 things both happen, we are deciding, for this "manual" stream-copy loop, we do want to stop feeding the stream copy (even though "the system" has enabed ignore_user_abort()). Because we know that it is pointless to keep looping and pushing out chunk-size bytes each time.

yevon · 2023-07-23T10:43:08Z

If the system setting is ignored (ignore_user_abort) why is it even checked? It should abort in both cases and with just1 === connection_aborted() is not enough? It is aborted automatically if system setting is enabled?

phil-davis · 2023-07-23T14:23:52Z

connection_aborted() is not enough?

That is probably correct. But the extra check is not going to break anything (or cause any different behavior, IMO!). So I guess we leave the code as it is.

proxima235 · 2023-08-08T17:53:14Z

I've checked out these fixes are working on my instance. But they don't make any changes in the case of link-sharing. My Nextcloud does not stop reading the file when accessed via a public link.
How can I make these fixes cover the case of link-sharing?

staabm · 2023-08-08T18:35:46Z

IMO the PR only works with chunked requests or range requests.

How do your http requests look like?

szaimen mentioned this pull request May 24, 2023

break stream if connection got aborted #205

Closed

kesselb force-pushed the handle-connection-abort-with-ignore-user-abort branch 3 times, most recently from 7ae9f26 to 1df4a84 Compare May 24, 2023 21:35

FedericoHeichou mentioned this pull request Jun 20, 2023

Huge download: File gets completely read even after download is cancelled nextcloud/server#8161

Open

staabm approved these changes Jun 21, 2023

View reviewed changes

phil-davis force-pushed the handle-connection-abort-with-ignore-user-abort branch from 1df4a84 to f2a1635 Compare June 26, 2023 09:01

Declare return type of stream_read in test

c574923

phil-davis reviewed Jun 26, 2023

View reviewed changes

staabm approved these changes Jun 26, 2023

View reviewed changes

phil-davis approved these changes Jun 26, 2023

View reviewed changes

phil-davis merged commit 97c8184 into sabre-io:master Jun 26, 2023

phil-davis mentioned this pull request Jun 26, 2023

Patches for 6.0.1 #210

Merged

This was referenced Jun 26, 2023

Bump PHP dependencies owncloud/core#40849

Merged

[FR] Stop reading file from disk if user cancelled download owncloud/core#32887

Closed

High disk usage on client disconnection nextcloud/server#19567

Closed

kesselb deleted the handle-connection-abort-with-ignore-user-abort branch June 26, 2023 12:22

Ruakij reviewed Jul 22, 2023

View reviewed changes

yevon mentioned this pull request Jul 22, 2023

[Bug]: output_buffering = Off / 0 in php and sendfile lack support is destroying Nextcloud download speeds nextcloud/server#39384

Closed

8 tasks

phil-davis mentioned this pull request Jul 25, 2023

Configuration in Sapi.php involved with php-fpm processes produced by playing large video files. #214

Closed

This was referenced Jul 26, 2023

abort on disconnect in rex_response->sendFile() redaxo/redaxo#5749

Merged

mark connection_status() and connection_aborted() impure JetBrains/phpstorm-stubs#1560

Closed

come-nc mentioned this pull request Sep 4, 2023

[stable25] Detect aborted connection in OC\Files\View and stop writing data to the output buffer nextcloud/server#40233

Merged

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: handle client disconnect properly with ignore_user_abort true #207

fix: handle client disconnect properly with ignore_user_abort true #207

kesselb commented May 24, 2023 •

edited

Loading

codecov bot commented May 25, 2023 •

edited

Loading

yevon commented Jun 10, 2023

FedericoHeichou commented Jun 20, 2023

szaimen commented Jun 20, 2023

staabm left a comment •

edited

Loading

staabm commented Jun 21, 2023

phil-davis commented Jun 26, 2023

phil-davis commented Jun 26, 2023

phil-davis Jun 26, 2023

staabm Jun 26, 2023

staabm Jun 26, 2023

staabm Jul 29, 2023

staabm commented Jun 26, 2023

phil-davis commented Jun 26, 2023

phil-davis commented Jun 26, 2023

yevon commented Jul 21, 2023

FedericoHeichou commented Jul 21, 2023 •

edited

Loading

yevon commented Jul 21, 2023 •

edited

Loading

FedericoHeichou commented Jul 21, 2023

szaimen commented Jul 21, 2023

Ruakij Jul 22, 2023

phil-davis Jul 23, 2023

yevon commented Jul 23, 2023 •

edited

Loading

phil-davis commented Jul 23, 2023

proxima235 commented Aug 8, 2023

staabm commented Aug 8, 2023

fix: handle client disconnect properly with ignore_user_abort true #207

fix: handle client disconnect properly with ignore_user_abort true #207

Conversation

kesselb commented May 24, 2023 • edited Loading

codecov bot commented May 25, 2023 • edited Loading

Codecov Report

yevon commented Jun 10, 2023

FedericoHeichou commented Jun 20, 2023

szaimen commented Jun 20, 2023

staabm left a comment • edited Loading

Choose a reason for hiding this comment

staabm commented Jun 21, 2023

phil-davis commented Jun 26, 2023

phil-davis commented Jun 26, 2023

phil-davis Jun 26, 2023

Choose a reason for hiding this comment

staabm Jun 26, 2023

Choose a reason for hiding this comment

staabm Jun 26, 2023

Choose a reason for hiding this comment

staabm Jul 29, 2023

Choose a reason for hiding this comment

staabm commented Jun 26, 2023

phil-davis commented Jun 26, 2023

phil-davis commented Jun 26, 2023

yevon commented Jul 21, 2023

FedericoHeichou commented Jul 21, 2023 • edited Loading

yevon commented Jul 21, 2023 • edited Loading

FedericoHeichou commented Jul 21, 2023

szaimen commented Jul 21, 2023

Ruakij Jul 22, 2023

Choose a reason for hiding this comment

phil-davis Jul 23, 2023

Choose a reason for hiding this comment

yevon commented Jul 23, 2023 • edited Loading

phil-davis commented Jul 23, 2023

proxima235 commented Aug 8, 2023

staabm commented Aug 8, 2023

kesselb commented May 24, 2023 •

edited

Loading

codecov bot commented May 25, 2023 •

edited

Loading

staabm left a comment •

edited

Loading

FedericoHeichou commented Jul 21, 2023 •

edited

Loading

yevon commented Jul 21, 2023 •

edited

Loading

yevon commented Jul 23, 2023 •

edited

Loading