Document that the calling code is responsible for closing streams

[andrewnicols]

Guzzle version(s) affected: 7.5.0
PHP version: 8.1.17
cURL version: 8.0.1.

Description
When passing in a stream as the contents argument to a multipart request, the stream is not closed by Guzzle.
As a result, it is not possible to remove the source file that the stream is opening in some OSes (e.g. Windows).

Arguably it's correc that it's not closed, but this should probably be noted in the documentation that the calling code is responsible for closing the stream when finished.

How to reproduce

This is easy to reproduce on Windows with unit tests where a file handle is not closed, and a file path is reused in a subsequent test. See the testcase here:

https://github.com/andrewnicols/guzzleCloseStreams
https://github.com/andrewnicols/guzzleCloseStreams/blob/main/tests/StreamedContentTest.php

This uses a data provider to run the same test with the same stream source five times.
The first two tests close the stream after running the test.
The third test does not.
The fourth test closes it, and the fifth test does not.

You can see in this run: https://github.com/andrewnicols/guzzleCloseStreams/actions/runs/4879658612/jobs/8706434793

The first three tests pass.
All tests after this point fail because the file handle in the third test was not closed.

Possible Solution
Document a note that the calling code is responsible for closing the stream.

Additional context
Seems to only affect Windows.

[mfn]

Interesting, in a different use-cse when using a sink and providing a "resource", once the Request object gets out of scope, the handle is closed => #2400

[GrahamCampbell]

Streams are, in general, closed automatically https://github.com/guzzle/psr7/blob/2.5.0/src/Stream.php#L69-L75, unless they stay referenced by your code or a previous response middleware. The test you have there is invalid, because the mock handler retains a reference to the stream.

[andrewnicols]

Hi Graham,

I have added an additional test to:

• not store the Mock in a variable, at all.
• expicitly unset the ref to the client.

Still failing:
https://github.com/andrewnicols/guzzleCloseStreams/blob/main/tests/StreamedContentTest.php#L104-L137
https://github.com/andrewnicols/guzzleCloseStreams/actions/runs/4911531600/jobs/8769649762

This behaviour should, at the very least, be documented.

[andrewnicols]

Hi @GrahamCampbell,

Just wanted to follow up with you on this one. This really should be documented - especially given it mostly seems to affect mocks badly -- even when there is no reference kept to them in the calling code.

[dotdash]

I've run into a similar issue. The problem here seems to be caused by reference cycles.

I added the following to Stream::__destruct() to see when the destructor gets called.

        var_dump(stream_get_meta_data($this->stream)['stream_type']);

I also removed the data provider to get less noise and added markers for where the unset call happens, with that I get:

$ vendor/bin/phpunit --filter UnsetMock 
PHPUnit 10.1.2 by Sebastian Bergmann and contributors.

Runtime:       PHP 8.2.12
Configuration: /home/bsteinbrink/src/guzzleCloseStreams/phpunit.xml

string(4) "TEMP"
string(12) "Before unset"
string(11) "After unset"
.                                                                   1 / 1 (100%)

Time: 00:00.005, Memory: 8.00 MB

OK (1 test, 4 assertions)
string(4) "TEMP"
string(4) "TEMP"
string(4) "TEMP"
string(5) "STDIO"

The TEMP ones are internally used streams, the STDIO one is the one for the file.

If I then add a call to gc_collect_cycles() after the unset($client) call, I get:

$ vendor/bin/phpunit --filter UnsetMock 
PHPUnit 10.1.2 by Sebastian Bergmann and contributors.

Runtime:       PHP 8.2.12
Configuration: /home/bsteinbrink/src/guzzleCloseStreams/phpunit.xml

string(4) "TEMP"
string(12) "Before unset"
string(11) "After unset"
string(4) "TEMP"
string(4) "TEMP"
string(4) "TEMP"
string(5) "STDIO"
string(12) "After cycles"
.                                                                   1 / 1 (100%)

Time: 00:00.005, Memory: 8.00 MB

OK (1 test, 4 assertions)

So with that call, the stream does get closed before the test method ends and it should succeed on Windows as well (I don't have a Windows box around to actually test it rn).

I'm not familiar with the guzzle code, so I didn't dig much further than that, and am not sure from where that cycle originates, but I hope this helps to clarify and maybe solve the issue.

Edit: I previously made a wrong statement about the destructor crashing when the stream has been manually closed, but there was a bug in my testing. While var_dump() still reports the stream as a resource, is_resource() will return false for it, and Stream::close() won't call fclose().

[dotdash]

Ok, so I did dig a bit deeper, and the cycle in the test case originates from the MockHandler keeping the last options and last request around. And these have references to the handler, so there's a cycle. Using a StreamHandler or CurlHandler instead, the streams are already closed before the unset($client) line.

@andrewnicols I suppose that you experienced this problem with something else than the MockHandler, right? I'm seeing this with AWS S3 uploads, and just now found aws/aws-sdk-php#2824