Skip to content

PHPLIB-1313 Ensure the GridFS stream is saved when the script ends #1197

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.

Sign up for GitHub

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

Merged
merged 2 commits into from
Nov 23, 2023
Merged

PHPLIB-1313 Ensure the GridFS stream is saved when the script ends #1197

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
e1eb3fd
PHPLIB-1313 Ensure the GridFS stream is saved when the script ends
GromNaN Nov 22, 2023
1697852
Update existing test instead of creating a new identical one
GromNaN Nov 23, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,4 +4,4 @@
## 1.18.0 (unreleased)

* Add `addSubscriber` and `removeSubscriber` methods to the `Client` class to ease dependency injection configuration

* Fix GridFS stream closing when the PHP script ends
9 changes: 5 additions & 4 deletions src/GridFS/StreamWrapper.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,10 +53,11 @@ class StreamWrapper

public function __destruct()
{
/* This destructor is a workaround for PHP trying to use the stream well
* after all objects have been destructed. This can cause autoloading
* issues and possibly segmentation faults during PHP shutdown. */
$this->stream = null;
Copy link
Member Author

@GromNaN GromNaN Nov 22, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can remove this line introduced by PHPLIB-345 because even if PHP tries to close the stream again, this is no-op.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note that the new test reproduces also the issue described in PHPLIB-345.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd have to go back and check, but PHP may also have fixed this issue by shutting down streams earlier in the shutdown process. Either way, manually closing the stream definitely fixes the write issue, but if we want to be absolutely sure we can also reset the stream property as was done previously to ensure the issue does not re-appear.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was able to reproduce the "autoloading" issue by removing the __destruct() function. So I don't see what would have changed in PHP core for this.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, I see. Since you've confirmed that the current code works around the issue, no objections to merging this as is 👍

/* Ensure the stream is closed so the last chunk is written. This is
* necessary because PHP would close the stream after all objects have
* been destructed. This can cause autoloading issues and possibly
* segmentation faults during PHP shutdown. */
$this->stream_close();
}

/**
Expand Down
29 changes: 21 additions & 8 deletions tests/GridFS/BucketFunctionalTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@
use function array_merge;
use function call_user_func;
use function current;
use function escapeshellarg;
use function exec;
use function fclose;
use function fopen;
Expand All @@ -39,7 +40,7 @@
use function strncasecmp;
use function substr;

use const PHP_EOL;
use const PHP_BINARY;
use const PHP_OS;

/**
Expand Down Expand Up @@ -853,21 +854,33 @@ public function testDanglingOpenWritableStream(): void
$this->markTestSkipped('Test does not apply to Windows');
}

$path = __DIR__ . '/../../vendor/autoload.php';
$command = <<<CMD
php -r "require '$path'; \\\$stream = (new MongoDB\Client)->test->selectGridFSBucket()->openUploadStream('filename', ['disableMD5' => true]);" 2>&1
CMD;
$code = <<<'PHP'
require '%s';
$client = new \MongoDB\Client(getenv('MONGODB_URI') ?: 'mongodb://127.0.0.1:27017/?serverSelectionTimeoutMS=100');
$database = $client->selectDatabase(getenv('MONGODB_DATABASE') ?: 'phplib_test');
$gridfs = $database->selectGridFSBucket();
$stream = $gridfs->openUploadStream('hello.txt', ['disableMD5' => true]);
fwrite($stream, 'Hello MongoDB!');
PHP;

@exec(
$command,
implode(' ', [
PHP_BINARY,
'-r',
escapeshellarg(sprintf($code, __DIR__ . '/../../vendor/autoload.php')),
'2>&1',
]),
$output,
$return,
);

$this->assertSame([], $output);
$this->assertSame(0, $return);
$output = implode(PHP_EOL, $output);

$this->assertSame('', $output);
$fileDocument = $this->filesCollection->findOne(['filename' => 'hello.txt']);

$this->assertNotNull($fileDocument);
$this->assertSame(14, $fileDocument->length);
}

/**
Expand Down