Skip to content

feat: Added async ticker#90

Merged
hsluoyz merged 1 commit intoapache:masterfrom
EmperorYP7:synced-enforcer
Apr 1, 2021
Merged

feat: Added async ticker#90
hsluoyz merged 1 commit intoapache:masterfrom
EmperorYP7:synced-enforcer

Conversation

@EmperorYP7
Copy link
Copy Markdown
Contributor

Signed-off-by: Yash Pandey (YP) yash.btech.cs19@iiitranchi.ac.in

This PR fixes #38

Reference: #89

Description

Most of the functions of SyncedEnforcer are straight-forward to implement but the AutoLoadPolicy needed a workaround since it utilized GoLang's ticker from the "time" module and concurrency used here.

ticker := time.NewTicker(d)

For this, I created a custom concurrent ticker that takes in the chrono::duration and a function<void()> for callback.
Ticker::start() will start a loop asynchronously (on a new thread). The parametric function will be called asynchronously as well.

Overheads involved:

Apart from the futures obtained from ticker threads, ticker also collects futures from callback at each tick since they're called asynchronously as well. This might lead to a pile of useless data given enough uptime and scope of the Ticker.

@EmperorYP7
Copy link
Copy Markdown
Contributor Author

I think Travis is stuck. Need to retrigger the build.
Screenshot 2021-03-30 at 10 19 28 PM

@hsluoyz
Copy link
Copy Markdown
Member

hsluoyz commented Mar 30, 2021

@EmperorYP7 can we fix this issue first? #91

@EmperorYP7
Copy link
Copy Markdown
Contributor Author

@hsluoyz sure!

@EmperorYP7
Copy link
Copy Markdown
Contributor Author

@EmperorYP7 can we fix this issue first? #91

@hsluoyz here it is: #92

Signed-off-by: Yash Pandey (YP) <yash.btech.cs19@iiitranchi.ac.in>
@hsluoyz
Copy link
Copy Markdown
Member

hsluoyz commented Mar 31, 2021

@xcaptain @divy9881

@xcaptain
Copy link
Copy Markdown

Looks good to me, just one question, if you are going to using this ticker to load policy automatically, do you still want to add a watcher or notify stuff.

@EmperorYP7
Copy link
Copy Markdown
Contributor Author

@xcaptain I think the Watcher and AutoLoadPolicy are mutually exclusive as implemented in enforcer_synced.go. Here's what I've planned to do:

// StartAutoLoadPolicy starts a thread that will go through every specified duration call LoadPolicy
 void SyncedEnforcer ::StartAutoLoadPolicy(chrono::duration<int64_t, std::nano> t) {
   // ...
   function<void()> onTick = [this]() {
     Enforcer::LoadPolicy();
     ++n;
   };
   ticker = unique_ptr<Ticker>(new Ticker(onTick, t));
   n = 1;
   ticker->start();
}
string SyncedEnforcer ::UpdateWrapper() {
   LoadPolicy();
   return "";
 }

 // SetWatcher sets the current watcher.
 void SyncedEnforcer ::SetWatcher(shared_ptr<Watcher> w){
   watcher = w;
   return watcher->SetUpdateCallback(&SyncedEnforcer::UpdateWrapper);

Although it seems redundant, this will be on par with the API in casbin. So according to me, the ticker should be used exclusively for AutoLoadPolicy.

One more way to implement this is to modify the watcher to utilize ticker and the StartAutoLoadPolicy would rely on this watcher for loading policy asynchronously.

@hsluoyz hsluoyz merged commit 6baafaa into apache:master Apr 1, 2021
@github-actions
Copy link
Copy Markdown

github-actions bot commented Apr 1, 2021

🎉 This PR is included in version 1.14.0 🎉

The release is available on GitHub release

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.

Add synced and cached enforcer

3 participants