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

[Stream] STREAM_NOTIFY_COMPLETED over HTTP never emitted #8641

Closed
sfaut opened this issue May 27, 2022 · 9 comments
Closed

[Stream] STREAM_NOTIFY_COMPLETED over HTTP never emitted #8641

sfaut opened this issue May 27, 2022 · 9 comments

Comments

@sfaut
Copy link

sfaut commented May 27, 2022

Description

The following code:

<?php

function progress($notification_code, $severity, $message, $message_code, $bytes_transferred, $bytes_maximum)
{
    printf(
        "%d\t%s\t%d\t%d\r\n",
        $notification_code,
        $notification_code == STREAM_NOTIFY_COMPLETED ? 'T' : 'F',
        $bytes_transferred, $bytes_maximum,
    );
};

$context = stream_context_create();
stream_context_set_params($context, ['notification' => 'progress']);
$stream_source = fopen('https://datasets.imdbws.com/title.ratings.tsv.gz', 'r', false, $context); // 6 MB file
$stream_destination = fopen('title.ratings.tsv.gz', 'w');
stream_copy_to_stream($stream_source, $stream_destination);

Resulted in this output:

2       F       0       0
4       F       0       0
5       F       0       6223383
7       F       0       6223383
7       F       8192    6223383
7       F       16384   6223383
7       F       24576   6223383
...
7       F       6207960 6223383
7       F       6216152 6223383
7       F       6223383 6223383

But I expected this output instead:

2       F       0       0
4       F       0       0
5       F       0       6223383
7       F       0       6223383
7       F       8192    6223383
7       F       16384   6223383
7       F       24576   6223383
...
7       F       6207960 6223383
7       F       6216152 6223383
8       T       6223383 6223383

With the last $notification_code = 8 as "STREAM_NOTIFY_COMPLETED".

Note that the last row has not always $bytes_transferred == $bytes_maximum

Thx :)

PHP Version

PHP 8.1.6

Operating System

Windows 10

@cmb69
Copy link
Member

cmb69 commented May 30, 2022

From a quick grep across the PHP-8.1 codebase, STREAM_NOTIFY_COMPLETED is never used. Not sure how to classify this ticket: bug or feature request.

@sfaut
Copy link
Author

sfaut commented Jun 5, 2022

STREAM_NOTIFY_COMPLETED is given as example in https://www.php.net/manual/function.stream-notification-callback
and should be implemented. Imho it should be considered like a bug.

Thx for your support,

@iluuu1994
Copy link
Member

I've looked through old versions and this was never implemented, so I think this qualifies as a feature request and/or documentation issue.

@cmb69
Copy link
Member

cmb69 commented Jun 9, 2022

Yeah, probably wouldn't make sense to treat it as bug, given it was never implemented, although I wonder why STREAM_NOTIFY_COMPLETED is there, and even has been documented.

@aetonsi
Copy link

aetonsi commented Dec 1, 2022

the last row has not always $bytes_transferred == $bytes_maximum

Since STREAM_NOTIFY_PROGRESS is not always emitted correctly (see #10031) and STREAM_NOTIFY_COMPLETED is never emitted, currently there's no way (from inside the notification callback) to determine if the download was completed. You have to check if the fetch operation (fopen, file_get_contents, etc) errored out or not.

@nielsdos
Copy link
Member

nielsdos commented Feb 2, 2023

I worked on an implementation of this feature and ran into #10031 as well (the completed event now triggers twice sometimes), so I'm going to spend some time to debug that issue.

nielsdos added a commit to nielsdos/php-src that referenced this issue Feb 4, 2023
…r emitted

This adds support for the completed event. We also have to check for
stream->eof in the `stream->readfilters.head` else branch in order to
prevent double firing the completed event. The non-else branch already
has that check.
nielsdos added a commit to nielsdos/php-src that referenced this issue Feb 4, 2023
…r emitted

This adds support for the completed event. Since the read handler could
be entered twice towards the end of the stream we remember what the eof
flag was before reading so we can emit the completed event when the flag
changes to true.
nielsdos added a commit to nielsdos/php-src that referenced this issue May 6, 2023
…r emitted

This adds support for the completed event. Since the read handler could
be entered twice towards the end of the stream we remember what the eof
flag was before reading so we can emit the completed event when the flag
changes to true.
nielsdos added a commit that referenced this issue Jun 10, 2023
This adds support for the completed event. Since the read handler could
be entered twice towards the end of the stream we remember what the eof
flag was before reading so we can emit the completed event when the flag
changes to true.

Closes GH-10505.
@nielsdos
Copy link
Member

Fixed via GH-10505.

@aetonsi
Copy link

aetonsi commented Jun 12, 2023

thanks @nielsdos !

@sfaut
Copy link
Author

sfaut commented Mar 13, 2024

Great ! Thx a lot @nielsdos :)

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

Successfully merging a pull request may close this issue.

5 participants