Skip to content

Allow app to track callback change #2206

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

Merged
merged 5 commits into from
May 17, 2025

Conversation

Ten0
Copy link
Contributor

@Ten0 Ten0 commented Feb 18, 2025

This allows apps to answer the question "was I already called as part of handling this particular event?" from anywhere in the code, by tracking the callback counter.

In particular this is useful to simplify https://github.com/Ten0/homeassistant_python_typer/blob/1f8075ae6655a65aab093f20fa4624d6587a201b/README.md#what-to-be-careful-about

This allows apps to answer the question "was I already called as part of handling this particular event?" from anywhere in the code, by tracking the callback counter.

In particular this is useful to simplify https://github.com/Ten0/homeassistant_python_typer/blob/1f8075ae6655a65aab093f20fa4624d6587a201b/README.md#what-to-be-careful-about
@acockburn
Copy link
Member

Hi there - I took a look at this with a view to merging it in the next release as we discussed on discord, however I see a few issues with the code as it stands.

  1. Unfortunately, as part of the massive Pydantic changes we just merged, there are now conflicts with this PR. Not a huge issue as it's fairly small, however:
  2. The changes aren't thread safe. This would possibly get corrupted if 2 threads were running the same code concurrently (not usual but it can be configured that way) so it would need some kind of lock. Or, it might make more sense to move this counter to an attribute for the app's entity - they are all updated in AD's asynchronous core so wouldn't need locking for that reason.
  3. The counter updates are present in async_worker() but not called in worker(), which means it would only work if the callback is an asynchronous callback

There is still time to get this into 4.5 if you want to take a crack at resolving the above issues - let me know if you have any questions, or jump into discord and shout me and we can discuss.

@jsl12 jsl12 self-assigned this Apr 17, 2025
@jsl12 jsl12 added the enhancement New feature or request label Apr 18, 2025
@Ten0 Ten0 force-pushed the app_track_callback_change branch from 8ad2fb4 to 0ead998 Compare April 23, 2025 18:57
@Ten0
Copy link
Contributor Author

Ten0 commented Apr 23, 2025

Hey!
Thanks a lot for your time on this topic, and apologies for the delay here (a head trauma is slowing me down a little these days 😵‍💫).

The counter updates are present in async_worker() but not called in worker(), which means it would only work if the callback is an asynchronous callback

Unless I'm mistaken I was calling it in the version of your previous review: 9e31c9e#diff-1a121850c02feea540ea9012eb3fb5e99550e2ce5f654293b672d45144271079R1015

Were you referring to a different place?

The changes aren't thread safe. This would possibly get corrupted if 2 threads were running the same code concurrently (not usual but it can be configured that way) so it would need some kind of lock.

Ah I thought I was covered by the GIL on app.counter += 1. 😅
/me updates their understanding on how hard multi-threading in Python is
Is this ok?

Or, it might make more sense to move this counter to an attribute for the app's entity

I went for the approach of using the app's own lock. The state approach seems heavy (in particular on the reading end): The use-case is just:

def some_function(...):
	if app.callback_counter != self.last_seen_callback_counter:
		self.clear_some_caches()
		self.last_seen_callback_counter = app.callback_counter
	# Now process the things

Is that ok?

@acockburn
Copy link
Member

Thanks for the update, and hope you feel better soon!

I’m traveling on business for the next few days but I’ll review this when I get home.

The plan is still to get this into 4.5!

@acockburn acockburn merged commit c292e60 into AppDaemon:dev May 17, 2025
7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants