Skip to content

Conversation

Copy link
Contributor

Copilot AI commented Oct 11, 2025

Problem

In AsyncInternalEnforcer, there was an inconsistency in how watcher methods were handled. While specialized watcher methods like update_for_add_policy were properly checked for being coroutine functions and awaited when necessary, the fallback watcher.update() method was called directly without any async checking.

Example of the issue:

if callable(update_for_add_policy):
    if inspect.iscoroutinefunction(update_for_add_policy):
        await update_for_add_policy(sec, ptype, rule)  # ✓ Properly handled
    else:
        update_for_add_policy(sec, ptype, rule)
else:
    self.watcher.update()  # ✗ Called directly without async check

This caused "coroutine was never awaited" warnings when using async watchers that implement only the base update() method without specialized methods. The watcher would not be properly notified of policy changes.

Solution

Applied consistent async handling to all watcher.update() calls by checking if the method is a coroutine function before calling it:

if inspect.iscoroutinefunction(self.watcher.update):
    await self.watcher.update()
else:
    self.watcher.update()

This pattern was applied to 10 locations across the following methods:

  • save_policy() - fallback case
  • _add_policy() - fallback case
  • _add_policies() - fallback case
  • _update_policy() - direct call
  • _update_policies() - direct call
  • _update_filtered_policies() - direct call
  • _remove_policy() - fallback case
  • _remove_policies() - fallback case
  • _remove_filtered_policy() - fallback case
  • _remove_filtered_policy_returns_effects() - direct call

Testing

Added comprehensive test coverage with:

  • AsyncMinimalWatcher - a minimal async watcher that only implements the async update() method
  • test_async_minimal_watcher() - validates that all policy operations properly await async watchers

All 301 tests pass with no warnings about unawaited coroutines.

Benefits

  • Consistency: All watcher method calls now follow the same async handling pattern
  • Correctness: Async watchers are properly awaited, eliminating runtime warnings
  • Compatibility: Works seamlessly with both sync and async watcher implementations
  • Future-proof: Ensures compatibility with async watchers that don't implement specialized update methods

Fixes inconsistent async handling as described in the related issue.

Original prompt

This section details on the original issue you should resolve

<issue_title>Inconsistent async handling for watcher.update() method</issue_title>
<issue_description>Description:

In the _add_policy async method, there's an inconsistency in how watcher methods are handled. While update_for_add_policy is properly checked for being a coroutine function and awaited if necessary, the fallback watcher.update() method is called directly without checking if it's async.

Current behavior:

if callable(update_for_add_policy):
    if inspect.iscoroutinefunction(update_for_add_policy):
        await update_for_add_policy(sec, ptype, rule)
    else:
        update_for_add_policy(sec, ptype, rule)
else:
    self.watcher.update()  # Called directly without async check

Expected behavior:
The watcher.update() method should also be checked for being async and handled accordingly:

if callable(update_for_add_policy):
    if inspect.iscoroutinefunction(update_for_add_policy):
        await update_for_add_policy(sec, ptype, rule)
    else:
        update_for_add_policy(sec, ptype, rule)
else:
    if inspect.iscoroutinefunction(self.watcher.update):
        await self.watcher.update()
    else:
        self.watcher.update()

Rationale:

  • Consistency: All watcher methods should be handled uniformly
  • Correctness: If watcher.update() is async, calling it without await would not execute properly
  • Future-proofing: This ensures compatibility with both sync and async watcher implementations

Impact:
This could cause issues when using async watchers that don't implement update_for_add_policy but have an async update() method.</issue_description>

Comments on the Issue (you are @copilot in this section)

@hsluoyz @1812526510 can you check how Golang Casbin handles it? https://github.com/casbin/casbin

Fixes #405


💬 Share your feedback on Copilot coding agent for the chance to win a $200 gift card! Click here to start the survey.

@CLAassistant
Copy link

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.

@hsluoyz hsluoyz marked this pull request as ready for review October 11, 2025 16:51
Copilot AI changed the title [WIP] Fix inconsistent async handling for watcher.update() method Fix inconsistent async handling for watcher.update() method Oct 11, 2025
Copilot AI requested a review from hsluoyz October 11, 2025 16:53
Copilot finished work on behalf of hsluoyz October 11, 2025 16:53
@hsluoyz
Copy link
Member

hsluoyz commented Oct 11, 2025

@1812526510 can you check if this PR resolved your issue?

@devamin
Copy link

devamin commented Oct 13, 2025

up, I was suffering from this issue and provided a fix in this PR :
pycasbin/casbin#15

cc @hsluoyz

@hsluoyz
Copy link
Member

hsluoyz commented Oct 14, 2025

@devamin https://github.com/pycasbin/casbin is NOT the official repo from Casbin team. The official repo is this repo: https://github.com/casbin/pycasbin. You need to create PR in this repo.

@1812526510
Copy link

@hsluoyz Thanks for the fix! Confirmed it’s working on my side

@hsluoyz
Copy link
Member

hsluoyz commented Oct 14, 2025

@1812526510 which fix? Give a link?

@1812526510
Copy link

@hsluoyz The code in the branch copilot/fix-inconsistent-async-handling fixed my issue.
Sorry, this is my first time opening an issue, so I’m not very familiar with the full process.
Please let me know if there’s anything I can help with or contribute!

@hsluoyz hsluoyz changed the title Fix inconsistent async handling for watcher.update() method feat: fix inconsistent async handling for watcher.update() method Oct 14, 2025
@hsluoyz hsluoyz changed the title feat: fix inconsistent async handling for watcher.update() method feat: fix inconsistent async handling bug for watcher.update() API Oct 14, 2025
@hsluoyz hsluoyz merged commit 7875d02 into master Oct 14, 2025
16 of 18 checks passed
github-actions bot pushed a commit that referenced this pull request Oct 14, 2025
# [2.4.0](v2.3.0...v2.4.0) (2025-10-14)

### Features

* fix inconsistent async handling bug for watcher.update() API ([#406](#406)) ([7875d02](7875d02))
@github-actions
Copy link

🎉 This PR is included in version 2.4.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

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

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Inconsistent async handling for watcher.update() method

5 participants