Skip to content

Create FileWatcher when ConfigManagingActor runs #1074

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 1 commit into from
Sep 13, 2024
Merged

Create FileWatcher when ConfigManagingActor runs #1074

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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 RELEASE_NOTES.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,4 +14,4 @@

## Bug Fixes

<!-- Here goes notable bug fixes that are worth a special mention or explanation -->
- `ConfigManagingActor`: Fixed an issue where the actor was unable to process events after being restarted.
75 changes: 40 additions & 35 deletions src/frequenz/sdk/config/_config_managing.py
View file
Open in desktop
Copy link
Contributor

Choose a reason for hiding this comment

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

I think you should add a del file_watcher when the run finishes (including if an exception is raised), otherwise the file watcher internal task will keep running. Sadly it doesn't provide a stop() yet, I think we should add one, as using del is weird.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think it's no needed. The reference to the local file_watcher variable is gone once exiting the _run() method in the actor which will cause the file watcher internal task to be stopped when the object is collected. See here for more details

Copy link
Contributor

Choose a reason for hiding this comment

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

Finalization is complicated. It is very likely that __del__ is called, but not guaranteed (if there is a cycle for example). This is why ideally we should probably have a stop() method and be more explicit about it.

I'm fine with leaving this as is, although I think it is always safer to do things explicitly to avoid surprises.

Copy link
Contributor Author

@daniel-zullo-frequenz daniel-zullo-frequenz Sep 11, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I agree it's safer to explicitly delete the file_watcher then, will do it. I presume I'll need capture the error, delete file_watcher and re-raise the error

Original file line number Diff line number Diff line change
Expand Up @@ -52,13 +52,8 @@ def __init__(
if isinstance(config_path, pathlib.Path)
else pathlib.Path(config_path)
)
# FileWatcher can't watch for non-existing files, so we need to watch for the
# parent directory instead just in case a configuration file doesn't exist yet
# or it is deleted and recreated again.
self._file_watcher: FileWatcher = FileWatcher(
paths=[self._config_path.parent], event_types=event_types
)
self._output: Sender[abc.Mapping[str, Any]] = output
self._event_types: abc.Set[EventType] = event_types

def _read_config(self) -> abc.Mapping[str, Any]:
"""Read the contents of the configuration file.
Expand Down Expand Up @@ -90,32 +85,42 @@ async def _run(self) -> None:
"""
await self.send_config()

async for event in self._file_watcher:
# Since we are watching the whole parent directory, we need to make sure
# we only react to events related to the configuration file.
if not event.path.samefile(self._config_path):
continue

match event.type:
case EventType.CREATE:
_logger.info(
"%s: The configuration file %s was created, sending new config...",
self,
self._config_path,
)
await self.send_config()
case EventType.MODIFY:
_logger.info(
"%s: The configuration file %s was modified, sending update...",
self,
self._config_path,
)
await self.send_config()
case EventType.DELETE:
_logger.info(
"%s: The configuration file %s was deleted, ignoring...",
self,
self._config_path,
)
case _:
assert_never(event.type)
# FileWatcher can't watch for non-existing files, so we need to watch for the
# parent directory instead just in case a configuration file doesn't exist yet
# or it is deleted and recreated again.
file_watcher = FileWatcher(
paths=[self._config_path.parent], event_types=self._event_types
)

try:
async for event in file_watcher:
# Since we are watching the whole parent directory, we need to make sure
# we only react to events related to the configuration file.
if not event.path.samefile(self._config_path):
continue

match event.type:
case EventType.CREATE:
_logger.info(
"%s: The configuration file %s was created, sending new config...",
self,
self._config_path,
)
await self.send_config()
case EventType.MODIFY:
_logger.info(
"%s: The configuration file %s was modified, sending update...",
self,
self._config_path,
)
await self.send_config()
case EventType.DELETE:
_logger.info(
"%s: The configuration file %s was deleted, ignoring...",
self,
self._config_path,
)
case _:
assert_never(event.type)
finally:
del file_watcher
Loading