Skip to content
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

Stoppable behavior #102

Open
wants to merge 11 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 9 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ Yii2 Queue Extension Change Log

## 2.0.1 under development

- Enh #102: Stoppable behavior (zhuravljov)
- Bug #118: Synchronized moving of delayed and reserved jobs to waiting list (zhuravljov)
- Bug #112: Queue command inside module (tsingsun)
- Enh #116: Add Chinese Guide (kids-return)
Expand Down
107 changes: 107 additions & 0 deletions src/stoppable/Behavior.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,107 @@
<?php
/**
* @link http://www.yiiframework.com/
* @copyright Copyright (c) 2008 Yii Software LLC
* @license http://www.yiiframework.com/license/
*/

namespace yii\queue\stoppable;

use yii\caching\Cache;
use yii\di\Instance;
use yii\queue\ExecEvent;
use yii\queue\Queue;

/**
* Stoppable Behavior allows stopping scheduled jobs in a queue.
*
* It provides a [[stop()]] method to mark scheduled jobs as "stopped", that
* will prevent their execution.
*
* This behavior should be attached to the [[Queue]] component.
*
* @author Roman Zhuravlev <[email protected]>
* @since 2.0.1
*/
class Behavior extends \yii\base\Behavior
Copy link
Member

Choose a reason for hiding this comment

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

imo it should be yii\queue\behaviors\Stoppable instead of yii\queue\stoppable\Behavior, Behavior as a class name does not make sense to me.

Copy link
Member Author

Choose a reason for hiding this comment

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

It will be used in a configuration by long name only. As well as closure behavior. Readability shouldn't be impaired.

'queue' => [
    //...
    'as stoppable' => \yii\queue\stoppable\Behavior::class,
    'as closure' => \yii\queue\closure\Behavior::class,
],

I think to make separate behaviors namespace isn't good idea.

Copy link

Choose a reason for hiding this comment

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

It will be used in a configuration by long name only

Why do you think that people uses long names in configuration? I almost always use short names with use statements at the top of the file.

Copy link
Member Author

Choose a reason for hiding this comment

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

@rob006, I'm not sure. But this make sense for config files, which cover many class definitions from different namespaces.

Copy link
Member Author

Choose a reason for hiding this comment

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

Anyway, you will resolve name conflicts using namespace aliases. DbConnection, RedisConnection etc. So you can add alias:

use yii\queue\stoppable\Behavior as QueueStoppable;

Copy link

Choose a reason for hiding this comment

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

I talk no about name suffix. Every package class is grouped to namespace by its role, no by class type.

This is extremely inconsistent to be honest. We have yii\queue\serializers for serializers, but doing the same for behaviors is wrong? So now we have at least three conventions for naming behaviors:

  • yii\queue\cli\Verbose,
  • yii\queue\LogBehavior,
  • yii\queue\stoppable\Behavior.

We have Interface suffix for interfaces, Event suffix for events, but name for behavior is simply random.

Copy link
Member Author

@zhuravljov zhuravljov Oct 15, 2017

Choose a reason for hiding this comment

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

We have yii\queue\serializers for serializers, but doing the same for behaviors is wrong?

Yes, because serializer implements only one role. It is queue\serializers.

Behavior is queue middleware. And behaviors can solve many different functions. queue\stoppable\Behavior allows to stop a job, queue\closure\Behavior serializes closures, queue\LogBehavior writes logs, etc.

So now we have at least three conventions for naming behaviors

I renamed Verbose to VerboseBehavior, and now we have single naming rule:

yii\queue\<Role><Type>

yii\queue\LogBehavior: Role = queue\Log, Type = Behavior
yii\queue\closure\Behavior: Role = queue\closure\, Type = Behavior
yii\queue\stoppable\Behavior: Role = stoppable\, Type = Behavior
yii\queue\PushEvent: Role = queue\Push, Type = Event
etc.

Copy link

Choose a reason for hiding this comment

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

I renamed Verbose to VerboseBehavior, and now we have single naming rule:

You clearly have two naming conventions:

  1. yii\queue\<Role><Type>
  2. yii\queue\<role>\<Type>

Anyway, I just hate using general names for specific implementations. It kills readability and productivity. I often use "Search by class name" feature in IDE - it is useless if you name all behaviors as "Behavior". You can't just import such class because you probably will get conflicts and Behavior will be just meaningless if you find it in code.

Copy link
Member Author

Choose a reason for hiding this comment

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

This is one of common ways of naming:

  • yii\<mode>\Application
  • yii\<package>\Module
  • yii\gii\generators\<entity>\Generator
  • Illuminate\Redis\Connections\Connection
  • Illuminate\Database\Connection
  • <vendor>\<package>\Client

I often use "Search by class name" feature in IDE - it is useless if you name all behaviors as "Behavior".

I'm not sure about all IDEs, but PHPStorm is able to search class usage considering namespace.

Copy link
Contributor

Choose a reason for hiding this comment

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

This is not a pointless discussion.

In certain discussions there is no right or wrong, just different opinions.
In this particular case, the Yii2 staff could enrich the discussion with their opinions.

In my opinion, the best solution would be to maintain the naming pattern used in Yii2.
After all, it is an extension to a larger project and here is no place to innovate.

cc: @cebe @samdark

{
/**
* @var Cache|array|string the cache instance used to store stopped status.
*/
public $cache = 'cache';
/**
* @var bool option allows to turn status checking off in case a driver does not support it.
*/
public $checkWaiting = true;
/**
* @var Queue
* @inheritdoc
*/
public $owner;

/**
* @inheritdoc
*/
public function init()
{
parent::init();
$this->cache = Instance::ensure($this->cache, Cache::class);
}

/**
* @inheritdoc
*/
public function events()
{
return [
Queue::EVENT_BEFORE_EXEC => 'beforeExec',
];
}

/**
* @param ExecEvent $event
*/
public function beforeExec(ExecEvent $event)
{
$event->handled = $this->isStopped($event->id);
}

/**
* Sets stop flag.
*
* @param string $id of a job
* @return bool
*/
public function stop($id)
{
if (!$this->checkWaiting || $this->owner->isWaiting($id)) {
$this->markAsStopped($id);
return true;
}

return false;
}

/**
* @param string $id of a job
* @return bool
*/
protected function markAsStopped($id)
{
$this->cache->set(__CLASS__ . $id, true);
}

/**
* @param string $id of a job
* @return bool
*/
protected function isStopped($id)
{
if ($this->cache->exists(__CLASS__ . $id)) {
$this->cache->delete(__CLASS__ . $id);
return true;
}

return false;
}
}
70 changes: 70 additions & 0 deletions tests/stoppable/StoppableTest.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,70 @@
<?php
/**
* @link http://www.yiiframework.com/
* @copyright Copyright (c) 2008 Yii Software LLC
* @license http://www.yiiframework.com/license/
*/

namespace tests\stoppable;

use tests\app\SimpleJob;
use tests\TestCase;
use Yii;
use yii\caching\ArrayCache;
use yii\queue\stoppable\Behavior as Stoppable;
use yii\queue\sync\Queue as SyncQueue;

/**
* Stoppable Test
*
* @author Roman Zhuravlev <[email protected]>
*/
class StoppableTest extends TestCase
{
public function testStop()
{
$job = new SimpleJob(['uid' => uniqid()]);
$id = $this->getQueue()->push($job);
$this->getQueue()->stop($id);
$this->getQueue()->run();
$this->assertFileNotExists($job->getFileName());
}

public function testNotStop()
{
$job = new SimpleJob(['uid' => uniqid()]);
$this->getQueue()->push($job);
$this->getQueue()->run();
$this->assertFileExists($job->getFileName());
}

/**
* @return SyncQueue|Stoppable
*/
protected function getQueue()
{
if (!$this->_queue) {
$this->_queue = new SyncQueue([
'handle' => false,
'as stoppable' => [
'class' => Stoppable::class,
'cache' => ArrayCache::class,
],
]);
}
return $this->_queue;
}

private $_queue;

/**
* @inheritdoc
*/
protected function tearDown()
{
foreach (glob(Yii::getAlias("@runtime/job-*.lock")) as $fileName) {
unlink($fileName);
}
parent::tearDown();
}
}