Skip to content

[Finder] Use a lazyIterator to close files descriptors when no longer used #40040

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

Jump to bottom
Merged
merged 1 commit into from
Feb 12, 2021
Merged

[Finder] Use a lazyIterator to close files descriptors when no longer used #40040

merged 1 commit into from
Feb 12, 2021

Conversation

jderusse
Copy link
Member

Q A
Branch? 4.4
Bug fix? yes
New feature? no
Deprecations? no
Tickets fix #35508
License MIT
Doc PR -

The RecursiveDirectoryIterator class open the file on __construct.
Because we Inject an instance of RecursiveDirectoryIterator inside the \AppendIterator` class, php opens a lot of file even before iterating on it.

This PR adds a new LazyIterator class that instantiate the decorated class only when something starts iterating on it.
When the iteration is over, it unset the variable to close let the decorated class clean things (ie. close the files)

@carsonbot carsonbot added this to the 4.4 milestone Jan 30, 2021
@jderusse jderusse force-pushed the finder-lazy branch 2 times, most recently from b984368 to c1b3e9b Compare January 30, 2021 17:12
@jderusse jderusse changed the title Use a lazyintertor to close files descriptors when no longer used [Finder] Use a lazyIterator to close files descriptors when no longer used Jan 30, 2021
@carsonbot
Copy link

Hey!

This is.. this is amazing. Thank you!

I think @hurricane-voronin has recently worked with this code. Maybe they can help review this?

Cheers!

Carsonbot

@jderusse jderusse force-pushed the finder-lazy branch 2 times, most recently from 1788517 to cb94f87 Compare February 1, 2021 20:54
@ro0NL
Copy link
Contributor

ro0NL commented Feb 5, 2021 •
edited
Loading

im wondering if IteratorAggregate would be more simple to implement

eg. append(LazyIterator::create($callable))

class LazyIterator implements IteratorAggregate {
   getIterator() {
     yield from ($this->callback)();
   }
  static create($callback) {
     return new IteratorIterator(new self($callback));
  }
}

roughly :)

@jderusse
Copy link
Member Author

jderusse commented Feb 5, 2021 •
edited
Loading

im wondering if IteratorAggregate would be more simple to implement

That's not possible, AppendIterator::append expect a instance of Iterator (in fact, everything in Finder is based on Iterator)

edit: I missed the trick with IteratorIterator... let me give a try

@jderusse
Copy link
Member Author

jderusse commented Feb 5, 2021

Refactored to use IteratorAggregate .
Thank you for the good idea @ro0NL

ro0NL
ro0NL approved these changes Feb 5, 2021
View reviewed changes
Copy link
Contributor

@ro0NL ro0NL left a comment

Choose a reason for hiding this comment

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

👍🏻 for LazyIterator as feature also, given Finder::append() supports aggregates, eg. Finder::append(new LazyIterator($callable)) works.

@fabpot
Copy link
Member

fabpot commented Feb 12, 2021

Thank you @jderusse.

@fabpot fabpot merged commit dc20a31 into symfony:4.4 Feb 12, 2021
stof
stof reviewed Feb 12, 2021
View reviewed changes
src/Symfony/Component/Finder/Iterator/LazyIterator.php
/**
* @author Jérémy Derussé <jeremy@derusse.com>
*/
class LazyIterator implements \IteratorAggregate
Copy link
Member

Choose a reason for hiding this comment

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

should we mark it as @internal ?

Copy link
Member

Choose a reason for hiding this comment

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

I agree, see #40163

@xabbuh xabbuh mentioned this pull request Feb 12, 2021
Merged
@nicolas-grekas nicolas-grekas mentioned this pull request Feb 12, 2021
Merged
fabpot added a commit that referenced this pull request Feb 12, 2021
@fabpot
minor #40163 [Finder] mark the LazyIterator class as internal (xabbuh)
4626100 
This PR was merged into the 4.4 branch.

Discussion
----------

[Finder] mark the LazyIterator class as internal

| Q             | A
| ------------- | ---
| Branch?       | 4.4
| Bug fix?      | no
| New feature?  | no
| Deprecations? | no
| Tickets       | #40040 (comment)
| License       | MIT
| Doc PR        |

Commits
-------

4a2c996 mark the LazyIterator class as internal
This was referenced Mar 4, 2021
Merged
Merged
joshtrichards pushed a commit to joshtrichards/symfony-finder that referenced this pull request Apr 26, 2024
@fabpot
minor #40163 [Finder] mark the LazyIterator class as internal (xabbuh)
e495096 
This PR was merged into the 4.4 branch.

Discussion
----------

[Finder] mark the LazyIterator class as internal

| Q             | A
| ------------- | ---
| Branch?       | 4.4
| Bug fix?      | no
| New feature?  | no
| Deprecations? | no
| Tickets       | symfony/symfony#40040 (comment)
| License       | MIT
| Doc PR        |

Commits
-------

8e69df8 mark the LazyIterator class as internal
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@stof stof stof requested changes

@ro0NL ro0NL ro0NL approved these changes

@xabbuh xabbuh xabbuh left review comments

@fabpot fabpot fabpot approved these changes

Assignees
No one assigned
Labels
Bug Finder Status: Reviewed
Projects
None yet
Milestone
4.4
Development

Successfully merging this pull request may close these issues.
6 participants
@jderusse @carsonbot @ro0NL @fabpot @stof @xabbuh