config set_atomic

Discussion forum for Extension Writers regarding Extension Development.
Post Reply
User avatar
Ger
Recognised Extension Developer
Posts: 1592
Joined: Wed Jan 02, 2008 7:35 pm
Location: 192.168.1.100
Contact:

config set_atomic

Post by Ger » Tue Mar 06, 2018 9:55 am

My Feed Post Bot extension was denied. I have 2 questions about the issues. I'll make them separate topics because they aren't really related.

First up: I use a simple \phpbb\config::set() here: https://github.com/GerB/feedpostbot/blo ... r.php#L116
Kasimi tells me I should use the set_atomic() method here. Quite frankly I've never heard of it, but it seems to be intended to only set the new value if the current value matches the provided $old_value parameter.

I wonder why I should change it. I simply want to set the feedpostbot_locked to the current time here, no matter what. I could of course tell it to only change when the $old_value = 0 but that check has already passed if I get to that point. Moreover: I certainly DON'T want to change anything or go any further if $old_value != 0.

Is there some sort of guideline why I should change this?
My extensions: Simple CMS, Feed post bot, Avatar Resize, Modbreak, Magic OGP, Live topic update and Modern Quote
Newest: Quoted Where + anonymize

Like my work? Buy me a coffee to keep it coming. :ugeek:
-Available for custom work-

User avatar
kasimi
Extension Customisations
Extension Customisations
Posts: 2976
Joined: Sat Sep 10, 2011 7:12 pm
Location: Germany
Contact:

Re: config set_atomic

Post by kasimi » Tue Mar 06, 2018 12:11 pm

Ger wrote:
Tue Mar 06, 2018 9:55 am
I wonder why I should change it. I simply want to set the feedpostbot_locked to the current time here, no matter what.
Not quite. You want to update the value only if the current value is 0. A lock should be acquired atomically to guarantee exclusive access. Have a look at phpBB's lock class, which by the way, you may use for your purpose too: https://github.com/phpbb/phpbb/blob/rel ... ock/db.php

User avatar
Ger
Recognised Extension Developer
Posts: 1592
Joined: Wed Jan 02, 2008 7:35 pm
Location: 192.168.1.100
Contact:

Re: config set_atomic

Post by Ger » Tue Mar 06, 2018 12:23 pm

kasimi wrote:
Tue Mar 06, 2018 12:11 pm
Ger wrote:
Tue Mar 06, 2018 9:55 am
I wonder why I should change it. I simply want to set the feedpostbot_locked to the current time here, no matter what.
Not quite. You want to update the value only if the current value is 0.
Yes, but like I wrote: I already checked that. If it isn't 0, execution is already interrupted here. I don't even want to get to that point. If the value isn't 0, I want to stop the entire process. Using set_atomic() doesn't prevent that, so I'd have to check anyway (what I already do).
So basically: if I get to line 116, I want to lock unconditionally.
Have a look at phpBB's lock class, which by the way, you may use for your purpose too: https://github.com/phpbb/phpbb/blob/rel ... ock/db.php
Never seen that one before actually. It seems to do somewhat the same though.

I wonder: is this a reason for denial or just a matter of opinion?
My extensions: Simple CMS, Feed post bot, Avatar Resize, Modbreak, Magic OGP, Live topic update and Modern Quote
Newest: Quoted Where + anonymize

Like my work? Buy me a coffee to keep it coming. :ugeek:
-Available for custom work-

User avatar
3Di
Registered User
Posts: 12749
Joined: Mon Apr 04, 2005 11:09 pm
Location: Milan (IT) Frankfurt (DE)
Name: Marco
Contact:

Re: config set_atomic

Post by 3Di » Tue Mar 06, 2018 6:20 pm

May I ask what's the purpose of it?
I am assuming you are bound to use a sort of cron job, or am I missing something?
Want to compensate me for my interest? Donate
Please PM me only to request paid works. Thx.
Extensions, Scripts, MOD porting, Update/Upgrades
My development's activity º PhpStorm's proud user

User avatar
kasimi
Extension Customisations
Extension Customisations
Posts: 2976
Joined: Sat Sep 10, 2011 7:12 pm
Location: Germany
Contact:

Re: config set_atomic

Post by kasimi » Tue Mar 06, 2018 10:34 pm

Ger wrote:
Tue Mar 06, 2018 12:23 pm
Yes, but like I wrote: I already checked that.
Not atomically though, which is what my initial suggestion is about.
https://en.wikipedia.org/wiki/Lock_(computer_science) wrote:The reason an atomic operation is required is because of concurrency, where more than one task executes the same logic. For example, consider the following C code:

Code: Select all

if(lock == 0) {
    // lock free, set it
    lock = myPID;
}
The above example does not guarantee that the task has the lock, since more than one task can be testing the lock at the same time.
I strongly suggest to use phpBB's lock class, as it uses a correct and tested locking algorithm and also has an automatic error recovery: if an error occurs during fetching feeds, i.e. while you have acquired the lock, it is releases after one hour, without having to reset it manually with your ACP button.

If you want to go with your own locking mechanism, this is how you could do it:

Code: Select all

if (!$this->config->set_atomic('feedpostbot_locked', 0, time(), false))
{
	// Feeds are already being fetched
	return;
}

// Fetch feeds

$this->config->set('feedpostbot_locked', 0, false);
Ger wrote:
Tue Mar 06, 2018 12:23 pm
I wonder: is this a reason for denial or just a matter of opinion?
If the issue is caught during testing it will result in a deny, as there would be duplicate feed items being posted. It's unlikely to occur in an isolated test environment with only one user, but it will cause problems on live boards eventually.

User avatar
Ger
Recognised Extension Developer
Posts: 1592
Joined: Wed Jan 02, 2008 7:35 pm
Location: 192.168.1.100
Contact:

Re: config set_atomic

Post by Ger » Wed Mar 07, 2018 8:03 am

@3Di: Yes, it's about an automated process through cron, while it can also be fetched manually through an ACP button.
kasimi wrote:
Tue Mar 06, 2018 10:34 pm
The above example does not guarantee that the task has the lock, since more than one task can be testing the lock at the same time.
If I'm not mistaken this can only happen when a cron job is running and a Admin triggers the job manually through the ACP in the *exact* same millisecond.

Also, I still don't understand what's the difference between this

Code: Select all

public function set_atomic($key, $old_value, $new_value, $use_cache = true)
{
    if (!isset($this->config[$key]) || $this->config[$key] == $old_value)
    {
	$this->config[$key] = $new_value;
	return true;
    }
    return false;
}
and this:

Code: Select all

$lock = (int) $this->config['feedpostbot_locked'];
if ($lock > 0)
{
        return 0;
}
// snip 6 irrelevant lines for purpose
$this->config->set('feedpostbot_locked', time());
Isn't it essentially the same, only more tailor-made? I only built in 6 lines in between for extra checks and settings which I want to be done before setting the lock.


On a side note: The $use_cache parameter seems unused in phpBB config.
My extensions: Simple CMS, Feed post bot, Avatar Resize, Modbreak, Magic OGP, Live topic update and Modern Quote
Newest: Quoted Where + anonymize

Like my work? Buy me a coffee to keep it coming. :ugeek:
-Available for custom work-

User avatar
kasimi
Extension Customisations
Extension Customisations
Posts: 2976
Joined: Sat Sep 10, 2011 7:12 pm
Location: Germany
Contact:

Re: config set_atomic

Post by kasimi » Wed Mar 07, 2018 10:21 am

This is the actual set_atomic() method used in production: https://github.com/phpbb/phpbb/blob/rel ... b.php#L137 $use_cache is used there, too.
Ger wrote:
Wed Mar 07, 2018 8:03 am
If I'm not mistaken this can only happen when a cron job is running and a Admin triggers the job manually through the ACP in the *exact* same millisecond.
I'm not familiar with your extension's architecture right now, but yes, it might be the only way to run into the problem. Low probability of it happening isn't a reason not to use set_atomic() though, I would argue. It takes minimal effort to fix it.

User avatar
spaceace
Registered User
Posts: 1796
Joined: Wed Jan 30, 2008 8:50 pm
Contact:

Re: config set_atomic

Post by spaceace » Wed Mar 07, 2018 11:05 am

sorry, wrong place :lol:

User avatar
Ger
Recognised Extension Developer
Posts: 1592
Joined: Wed Jan 02, 2008 7:35 pm
Location: 192.168.1.100
Contact:

Re: config set_atomic

Post by Ger » Wed Mar 07, 2018 12:19 pm

kasimi wrote:
Wed Mar 07, 2018 10:21 am
This is the actual set_atomic() method used in production: https://github.com/phpbb/phpbb/blob/rel ... b.php#L137 $use_cache is used there, too.
Hmmm... That's odd. Can you explain to me how this works?

In my constructor I inject \phpbb\config\config, yet in my services.yml I use '@config' which, as you point out, calls phpbb\config\db. Which class am I actually using in that case? And, if the first one isn't used, why does it exist? It's a bit confusing now.
And when I use the global $config var in the ACP module, which class is called?

Also: the $use_cache parameter is confusing to me.
$use_cache Whether this variable should be cached or if it changes too frequently to be efficiently cached
Yet the code says:

Code: Select all

if ($use_cache)
{
	$this->cache->destroy('config');
}
So if you want to use cache, the cache is destroyed? Isn't this exactly the opposite of what both the parameter name as it's description state?
I'm not familiar with your extension's architecture right now, but yes, it might be the only way to run into the problem. Low probability of it happening isn't a reason not to use set_atomic() though, I would argue. It takes minimal effort to fix it.
Aside from the effort (this whole discussion takes more effort than changing the code) I want to understand. I don't like to simply change some code because somebody says so, I need to know why it's better.
My extensions: Simple CMS, Feed post bot, Avatar Resize, Modbreak, Magic OGP, Live topic update and Modern Quote
Newest: Quoted Where + anonymize

Like my work? Buy me a coffee to keep it coming. :ugeek:
-Available for custom work-

User avatar
kasimi
Extension Customisations
Extension Customisations
Posts: 2976
Joined: Sat Sep 10, 2011 7:12 pm
Location: Germany
Contact:

Re: config set_atomic

Post by kasimi » Wed Mar 07, 2018 3:17 pm

phpbb\config\db extends phpbb\config\config. That's why your constructor accepts the former. The latter is used during installation and can also be useful for testing purposes, because it doesn't depend on a database providing the config values as it accepts them in its constructor. So, which class you're actually using depends on the environment.

The $use_cache argument should be set to false for dynamic config values, which your feedpostbot_locked should be, as clearing the cache for those isn't necessary. You can think of the argument as "is using the cache".
Ger wrote:
Wed Mar 07, 2018 12:19 pm
Aside from the effort (this whole discussion takes more effort than changing the code) I want to understand. I don't like to simply change some code because somebody says so, I need to know why it's better.
What it comes down to is that a lock needs to be acquired atomically to work in a reliable way. This is part of the definition of a lock and not an opinion or code style preference. See the Wiki for details and phpBB's lock class for a reference implementation, both linked above.

User avatar
Ger
Recognised Extension Developer
Posts: 1592
Joined: Wed Jan 02, 2008 7:35 pm
Location: 192.168.1.100
Contact:

Re: config set_atomic

Post by Ger » Thu Mar 08, 2018 8:34 am

kasimi wrote:
Wed Mar 07, 2018 3:17 pm
phpbb\config\db extends phpbb\config\config. That's why your constructor accepts the former. The latter is used during installation and can also be useful for testing purposes, because it doesn't depend on a database providing the config values as it accepts them in its constructor. So, which class you're actually using depends on the environment.
I see. So actually it would be better to specify in it as \phpbb\config\config $config in my constructor, as that is what's actually injected.
My extensions: Simple CMS, Feed post bot, Avatar Resize, Modbreak, Magic OGP, Live topic update and Modern Quote
Newest: Quoted Where + anonymize

Like my work? Buy me a coffee to keep it coming. :ugeek:
-Available for custom work-

User avatar
kasimi
Extension Customisations
Extension Customisations
Posts: 2976
Joined: Sat Sep 10, 2011 7:12 pm
Location: Germany
Contact:

Re: config set_atomic

Post by kasimi » Thu Mar 08, 2018 10:30 am

Ger wrote:
Thu Mar 08, 2018 8:34 am
I see. So actually it would be better to specify in it as \phpbb\config\config $config in my constructor, as that is what's actually injected.
Did you mean \phpbb\config\db $config there? :P I prefer using \phpbb\config\config as my code doesn't require the config values to come from the database, but this is entirely up to you.

User avatar
Ger
Recognised Extension Developer
Posts: 1592
Joined: Wed Jan 02, 2008 7:35 pm
Location: 192.168.1.100
Contact:

Re: config set_atomic

Post by Ger » Thu Mar 08, 2018 10:44 am

kasimi wrote:
Thu Mar 08, 2018 10:30 am
this is entirely up to you.
Alright then. Thanks.
My extensions: Simple CMS, Feed post bot, Avatar Resize, Modbreak, Magic OGP, Live topic update and Modern Quote
Newest: Quoted Where + anonymize

Like my work? Buy me a coffee to keep it coming. :ugeek:
-Available for custom work-

Post Reply

Return to “Extension Writers Discussion”

Who is online

Users browsing this forum: No registered users and 14 guests