When attempting to migrate image files from an external legacy website, I encountered the following error after processing about 1000 files:
[warning] include(.../docroot/core/modules/migrate/src/MigrateException.php): failed to open stream: Too many open files ClassLoader.php:444
In my implementation, I used the transform method on the "download" process plugin from within another custom process plugin like so:
$uri = $this->downloadPlugin->transform([
$source,
$destination,
], $migrate_executable, $row, $destination_property);
The error should trigger when the Download plugin's transform method is called about 1000 times.
It appears the Download process plugin fails to close the stream it opens when it downloads the file. This eventually results in triggering the error from the ClassLoader.
For testing purposes, I tried modifying the transform method within the Download plugin class to close the stream before exiting:
public function transform($value, MigrateExecutableInterface $migrate_executable, Row $row, $destination_property) {
...
@fclose($destination_stream);
return $final_destination;
}
By doing this the error was no longer triggered. As such, I think this issue might be resolved by simply closing the open stream. However, since the stream is saved to the configuration as $this->configuration['guzzle options']['sink'] I'm not sure how this change would effect code, if any, that might later require the stream resource.
| Comment | File | Size | Author |
|---|---|---|---|
| #24 | 3163663-24.patch | 2.73 KB | quietone |
| #24 | interdiff-22-24.txt | 2.45 KB | quietone |
| #22 | 3163663-22.test-only.patch | 872 bytes | alexpott |
| #21 | 3163663-21.test-only.patch | 768 bytes | alexpott |
| #7 | 3163663-migrate-too-many-open-files-bug-7.patch | 601 bytes | chrisolof |
Show commandsStart within a Git clone of the project using the version control instructions.
Or, if you do not have SSH keys set up on git.drupalcode.org:
changes
Comments
Comment #2
larowlanThanks, this is an issue in your local environment - you need to configure ulimits
See https://serverfault.com/questions/15564/where-are-the-default-ulimits-sp... for example
Comment #3
rzan commentedThank you for the response.
I understand the nature of limits within the local environment, but I created this issue to ask why the file stream is not closed after a download is complete. Increasing environment limits does resolve the issue, but an arbitrarily large file migration will inevitably overcome any finite limit.
If no limit is set, then a sufficiently large migration could eventually monopolize local resources. It is also good coding practice to close file streams after you are finished so that other processes may access them.
Is there any particular reason why the streams are kept open for the entire length of the migration?
Comment #4
larowlanThat is a good question. Sorry, I didn't see the proposed resolution last time
Back to active
Comment #5
chrisolofI just hit this myself today and rzan's solution of closing the open file pointer after downloading the remote file appears to work. I'm attaching a one-line patch against Drupal 8.9.x for review.
This bug also affects the file_copy plugin when used with remote source files, and the patch fixes that scenario too.
Comment #7
chrisolofNew patch checks that $destination_stream is not FALSE before attempting to close it.
Comment #10
mpp commented+ if ($destination_stream) {
+ fclose($destination_stream);
+ }
Imo, since the current code is doing @fopen we could silence the @close as well.
Comment #12
mpp commentedComment #15
chrisolof+ if ($destination_stream) {
+ @fclose($destination_stream);
+ }
I'm finding that on some systems $destination_stream is already closed by the time we get here. Presumably on these environments the bug we're discussing here would not present. Also on these systems, the
if ($destination_stream)check is insufficient. A closed resource would evaluate to TRUE, so we'll attempt to close it again, which will raise a PHP warning (which the@will suppress). I think this is what was tripping up the test result in #7.While the
@certainly silences the warning on these systems, I think we might want to replace the plain if statement withif (is_resource($destination_stream)). is_resource() will return FALSE for a closed resource, thus avoiding any attempts to close a resource that was already closed.At any rate it's a suggestion. The current patch / merge request in #14 certainly works well in my tests across both types of systems, but I do believe we're still trying to close an already-closed resource on certain systems.
Comment #16
mpp commentedThanks for the input @chrisolof. Not sure where in Download::transform the stream could have been closed prior to the fclose statement?
I applied both suggestions:
Let's see what the testbot has to say.
Comment #18
robertoperuzzoPatch #7 works for me on D9.1.5 deployed in Platform.sh
Comment #19
mikelutzThis seems obvious, and I can't think of any reasonable way to add a test around the fix, so RTBC
Comment #20
alexpottWe can use
get_resources()to determine the number of open resources. And therefore this is testable.Comment #21
alexpottHere's the test... this will fail. Someone can improve the comments around the test and add it to the MR and then we should be good to go.
Comment #22
alexpottOpps coding standards...
Comment #24
quietone commented@alexpott, thanks for writing a test.
Added a comment to the new test code. Shortened the additional comment because I think there is enough information in context. And added the fix.
Comment #25
quietone commentedMeant to add that I removed the message from the assertion because of #3131946: [policy] Remove PHPUnit assertion messages when possible, and standardize remaining messages .
Comment #26
mikelutzHa, I did a real quick Google on seeing if there was a method to see how many resources were open, and the result I saw didn't mention this method. Thanks, @alexpott I wanted there to be a method to make the test this easy, lol. Back to Rtbc!
Comment #27
alexpottCommitted and pushed 76b866462d to 9.4.x and 32c93c8b98 to 9.3.x. Thanks!
Backported to 9.3.x as this is a low-risk bug fix.