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

sink tmpfile() closed before it can be used (scoping issue) #2400

Open
mfn opened this issue Nov 14, 2019 · 20 comments
Open

sink tmpfile() closed before it can be used (scoping issue) #2400

mfn opened this issue Nov 14, 2019 · 20 comments
Labels
lifecycle/keep-open Issues that should not be closed

Comments

@mfn
Copy link

mfn commented Nov 14, 2019

Guzzle version(s) affected: 6.3.3

Description
When using the sink option and providing a tmpfile(), that handle is closed when the Response gets out of scope.

How to reproduce

<?php declare(strict_types=1);

use GuzzleHttp\RequestOptions;
use GuzzleHttp\Client;

require_once __DIR__ . '/vendor/autoload.php';

function tmpfile_already_closed()
{
    $tmpfile = tmpfile();

    $guzzle = new Client();
    $response = $guzzle->request('GET', 'http://example.com', [
        RequestOptions::SINK => $tmpfile,
    ]);
    var_dump($tmpfile);

    return $tmpfile;
}

function tmpfile_still_ok()
{
    $tmpfile = tmpfile();

    $guzzle = new Client();
    $response = $guzzle->request('GET', 'http://example.com', [
        RequestOptions::SINK => $tmpfile,
    ]);
    var_dump($tmpfile);

    return [$response, $tmpfile];
}

$tmpfile = tmpfile_already_closed();
var_dump($tmpfile);

[$response,$tmpfile] = tmpfile_still_ok();
var_dump($tmpfile);

Output on my system (osx, but the same on linux; I added the empty line for clarity):

sink-tmpfile.php:16:
resource(15) of type (stream)
sink-tmpfile.php:35:
resource(15) of type (Unknown)

sink-tmpfile.php:29:
resource(49) of type (stream)
sink-tmpfile.php:38:
resource(49) of type (stream)

The underlying problem is that when using sink a stream is used within \GuzzleHttp\Psr7\Response.

That stream is \GuzzleHttp\Psr7\Stream and has a __destruct implementation:

    public function __destruct()
    {
        $this->close();
    }
…
    public function close()
    {
        if (isset($this->stream)) {
            if (is_resource($this->stream)) {
                fclose($this->stream);
            }
            $this->detach();
        }
    }

This caught me by surprise and I would have loved to use the sink with tmpfile. Notwithstanding the debugging necessary to figure this out 😄

I eventually had to change the implementation to tempnam for now (this also uses streams but the reference is a filename).

No amount of googling brought this up so in this interest I created this issue.

Not sure what a fix could be as I don't have insights why there is even a manual fclose here.

I would have expected:

  • the application provides the resource
  • the application manages the resource

But Guzzlehttp interfered here.

@Nyholm
Copy link
Member

Nyholm commented Nov 18, 2019

Hm.. I understand this is a bit weird? But what would a fix be? Not close the stream and leave the resource open? That is no good..

@mfn
Copy link
Author

mfn commented Nov 18, 2019

I guess my point is that the "creator" of the resource is also responsible for managing (=closing) it, and not the library working with it.

@sergejostir
Copy link

Any progress on this?

@mfn
Copy link
Author

mfn commented Mar 27, 2020

I don't have more information than presented in this issue.

@aarondfrancis
Copy link

@mfn I think I may have a workaround for you. Seems to be working for us:

$tmp = tmpfile();

// Assign the response to a variable to keep the tmpfile open
$response = $guzzle->request('GET', 'http://example.com/csv/', [
    'sink' => $tmp
]);

// The we pass it straight into a CSV reader.
$reader = Reader::createFromStream($tmp);
        

@mfn
Copy link
Author

mfn commented Apr 21, 2020

Thanks, but I'm aware.

However my use-case requires processing this data somewhere else where the response is already out of scope!

@aarondfrancis
Copy link

Ok, sorry.

@salimkanoun
Copy link

hi there,
I'm facing the same issue, can't use tmpfile with guzzle because of the ressource closing by guzzle.

No plan to make the ressource closing optional or something ?
tmpfile are helpfull because because they are removed automatically at script interuption, would be great to use it with guzzle,

Best regards,

Salim

@stale
Copy link

stale bot commented Dec 7, 2020

This issue has been automatically marked as stale because it has not had recent activity. It will be closed after 2 weeks if no further activity occurs. Thank you for your contributions.

@stale stale bot added the lifecycle/stale No activity for a long time label Dec 7, 2020
@mfn
Copy link
Author

mfn commented Dec 7, 2020

Nothing changed, behaviour is still the same with:

  • Guzzle 7.2.0
  • PHP 7.4.13

See example from initial description:

$ php guzzle_2400.php
resource(118) of type (stream)
resource(118) of type (Unknown)
resource(159) of type (stream)
resource(159) of type (stream)

@stale stale bot removed the lifecycle/stale No activity for a long time label Dec 7, 2020
@mylemans
Copy link

@mfn I have no experience with this in PHP, but can't you simply wrap the tmpfile resource in a custom 'Stream' that simply does not do the fclose bit on __destruct and feed that to Guzzle?

@mfn
Copy link
Author

mfn commented Apr 1, 2021

Interesting idea, I tried it in 5 mins but it went nowhere. Also, the StreamInterface has an awful lot of methods to implement (> 10) 😢

When I pass StreamInterface as sink, it only calls __toString() and nothing else on the interface. But the stream is supposed to be written to, so 🤷‍♀️

@stale
Copy link

stale bot commented Jul 30, 2021

This issue has been automatically marked as stale because it has not had recent activity. It will be closed after 2 weeks if no further activity occurs. Thank you for your contributions.

@stale stale bot added the lifecycle/stale No activity for a long time label Jul 30, 2021
@mfn
Copy link
Author

mfn commented Aug 5, 2021

not stale

@stale stale bot removed the lifecycle/stale No activity for a long time label Aug 5, 2021
@stale
Copy link

stale bot commented Dec 4, 2021

This issue has been automatically marked as stale because it has not had recent activity. It will be closed after 2 weeks if no further activity occurs. Thank you for your contributions.

@stale stale bot added the lifecycle/stale No activity for a long time label Dec 4, 2021
@mfn
Copy link
Author

mfn commented Dec 4, 2021

not stale

@stale stale bot removed the lifecycle/stale No activity for a long time label Dec 4, 2021
@stale
Copy link

stale bot commented Apr 3, 2022

This issue has been automatically marked as stale because it has not had recent activity. It will be closed after 2 weeks if no further activity occurs. Thank you for your contributions.

@stale stale bot added the lifecycle/stale No activity for a long time label Apr 3, 2022
@GrahamCampbell GrahamCampbell added the lifecycle/keep-open Issues that should not be closed label Apr 3, 2022
@stale stale bot removed the lifecycle/stale No activity for a long time label Apr 3, 2022
@mfn
Copy link
Author

mfn commented Apr 4, 2022

Thank you @GrahamCampbell !

@GrahamCampbell
Copy link
Member

Thank me when it's fixed, lol. ;)

@mfn
Copy link
Author

mfn commented Apr 5, 2022

lol, looking forward to it! (i.e. the acceptable solution)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
lifecycle/keep-open Issues that should not be closed
Projects
None yet
Development

No branches or pull requests

7 participants