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

BUGFIX: Stabilise file monitor #3304

Draft
wants to merge 2 commits into
base: 9.0
Choose a base branch
from

Conversation

mhsdesign
Copy link
Member

While trying to fix #3303 i noticed that my file monitor had stale caches.

I deleted a file long time, but it was still present in Cache/Data/Flow_Monitor/Flow_ClassFiles_filesAndModificationTimes.
That should not be the case as it should have been flushed. Maybe the file monitor even noticed the change and other parts of flow were aware of it but just the deletion in the file didnt work.

Either way it seems to be flaky and expensive to have two caches with similar data.
This change in theorie makes the Flow_ClassFiles_directoriesAndFiles cache obsolete by introducing a new method to that on the ModificationTimeStrategy.

The idea was, if i loop already through all the folders we can just diff it to the previously saved modification stamps and see whats left and check for file existence.

That seems to work well. And in one hickup in flow, that i could not further reproduce, even better than the current strategy which didnt detect a removal.

to test i just dumped a bit around ;D

if ($this->identifier === 'Flow_ClassFiles' && str_ends_with($path, '/Packages/Neos/Neos.Neos/Classes/')) {
    // previously considered files for deletion
    var_dump($this->directoriesAndFiles[$path]);
    // more relivable?
    var_dump($deletedFiles);
}

TODOS;

  • why is the filemonitor proxied???
  • do people implement their own strategy? at least this commit suggests that: kitsunet@b54f803

if ($this->identifier === 'Flow_ClassFiles' && str_ends_with($path, '/Packages/Neos/Neos.Neos/Classes/')) {
    // previously considered files for deletion
    var_dump($this->directoriesAndFiles[$path]);
    // more relivable?
    var_dump($deletedFiles);
}
@github-actions github-actions bot added the 9.0 label Feb 4, 2024
@mhsdesign mhsdesign marked this pull request as ready for review February 6, 2024 19:37
@mhsdesign mhsdesign changed the title WIP Bugfix: stabilise file monitor BUGFIX: Stabilise file monitor Feb 6, 2024
@github-actions github-actions bot added the Bug label Feb 6, 2024
@mhsdesign
Copy link
Member Author

I discussed this shortly with @kitsunet this seems a more stable way. Separating the FileMonitor from its ChangeDetectionStrategyInterface is a doomed task and probably doesn't even work currently, that means no one can implement this.
So we dont need to make sure that things are backwards compatible.

@kitsunet
Copy link
Member

IMHO we should internal or deprecate the interface though, to make this clear?

@mhsdesign mhsdesign marked this pull request as draft March 28, 2024 20:44
@mhsdesign
Copy link
Member Author

Yes id say we have ONE internal ChangeDetectionStrategyInterface which will behave like proposed and remove all these funny strategies.

If you still agree - at least thats what we talked about once see #3304 (comment) - please let me know then ill refactor this ;)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

BUG: Reflection cache for class is not flushed on file deletion
2 participants