Skip to content

Conversation

@daniel-zullo-frequenz
Copy link
Contributor

The ConfigManagingActor was unable to process events after being restarted because the FileWatcher cannot be reused when the ConfigManagingActor is restarted, so we need to create a new instance every time the actor starts running.

@daniel-zullo-frequenz daniel-zullo-frequenz requested a review from a team as a code owner September 10, 2024 14:55
@daniel-zullo-frequenz daniel-zullo-frequenz requested review from llucax and removed request for a team September 10, 2024 14:55
@github-actions github-actions bot added part:docs Affects the documentation part:config Affects the configuration management labels Sep 10, 2024
Copy link
Contributor

@llucax llucax left a comment

Choose a reason for hiding this comment

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

Other than that, LGTM.

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

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

@daniel-zullo-frequenz
Copy link
Contributor Author

Updated to explicitly delete file_watcher when exiting _run(). I needed to capture BaseException to be able to delete file_watcher when the actor is stopped (by calling stop()).

Comment on lines 125 to 127
except (Exception, BaseException):
del file_watcher
raise
Copy link
Contributor

Choose a reason for hiding this comment

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

You are manually writing a finally 😆

Suggested change
except (Exception, BaseException):
del file_watcher
raise
finally:
del file_watcher

You could also use a ExitStack. Or we could make the FileWatcher a context manager (look ma, here it comes BackgroundService/Service again, if FileWatcher were a Service it would automatically be a context manager 😉), but I would leave that for the future as it requires a lot of work (changing FileWatcher, releasing etc.) and it should come automatically when we migrate channels to use core (and Service).

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 really forgot finally exists in python, somehow I only thought of try/except/else...sorry 👼

You could also use a ExitStack. Or we could make the FileWatcher a context manager (look ma, here it comes BackgroundService/Service again, if FileWatcher were a Service it would automatically be a context manager 😉), but I would leave that for the future as it requires a lot of work (changing FileWatcher, releasing etc.) and it should come automatically when we migrate channels to use core (and Service).

Agreed

The ConfigManagingActor was unable to process events
after being restarted because the FileWatcher cannot
be reused when the ConfigManagingActor is restarted,
so we need to create a new instance every time the
actor starts running.

Signed-off-by: Daniel Zullo <[email protected]>
@daniel-zullo-frequenz daniel-zullo-frequenz added this pull request to the merge queue Sep 13, 2024
Merged via the queue into frequenz-floss:v1.x.x with commit 9f4681e Sep 13, 2024
18 checks passed
@daniel-zullo-frequenz daniel-zullo-frequenz deleted the fix/config-manager branch September 13, 2024 10:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

part:config Affects the configuration management part:docs Affects the documentation

Projects

Development

Successfully merging this pull request may close these issues.

2 participants