記事の公開・非公開日時処理のロジックについて（指定時間以降に記事が公開されない）

[shobu]

いつもお世話になっております。

以下、手元の環境でテストした結果なのですが、皆さまの環境も同様でしょうか？
ソースファイル的にはテスト結果通りになると思われますが…。

※本来はunixtime表記ですが、分かりづらいので実時間で書いています。

現時点が 2013/7/31 12:00:00 。この時点で公開非公開設定された内容は無いものとします。

一つ目のドキュメントについて、公開日時を 2013/7/31 15:00:00 として保存するとき、他の公開非公開イベントが無ければ、 sitePublishing.idx.php の中身の時間は 2013/7/31 15:00:00 となります。

この次に二つ目のドキュメントの公開日時を 2013/7/31 13:00:00 として保存すると、一番直近のイベントはこのドキュメントになるので、 sitePublishing.idx.php の中身の時間は 2013/7/31 13:00:00 となります。

その後、少し時間が経ち 2013/7/31 14:00:00 を過ぎたとします。
ここまで外部ユーザのサイト閲覧、管理者のプレビュー、キャッシュクリアが一切無い場合、前述の二つ目 13:00:00公開予定の記事は未公開のままです。

この時点を14:05ぐらいとしますが、一つ目のドキュメントを再保存すると、 sitePublishing.idx.php の中身の時間は 2013/7/31 15:00:00 となります。
この状態になると、次のキャッシュクリア・公開非公開処理がfrontで走るのは15:00ということになるので、再保存時点から15:00:00までの間、14:00:00公開予定の二つめの記事の公開処理が走らなくなります。

ソースファイルを見ると、古いものとチャンクで公開日時設定が出来るようになった最新版で若干違いますが、処理のロジックは同じで

/manager/processors/cache_sync.class.processor.php publish_time_file 関数に

$result = $modx->db->select('MIN(pub_date) AS minpub','[+prefix+]site_content', "{$current_time} < pub_date");

この様な感じの処理があります。modx_site_contentの中で最も小さなpub_dateを取得。「ただし、現時点より後」という条件です。
実際にはチャンクなども含めて公開・非公開日時も取得して最小値を求めていますが、全て「ただし、現時点より後」と条件があり、これが前述の問題を引き起こしていると思います。

とりあえずキャッシュクリアを定期的に走らせるという対処療法で誤魔化していますが、根本的に修正しないといけない内容ではないかと思いますがいかがでしょうか。

今の所、代替の良い処理、条件が思いついていません。
何か良いお知恵があれば…。

[shobu]

度々スミマセン。

ソースまで確認していませんが、同じような処理をしていると思われる、管理画面の「公開状態の一覧」も非公開のまま予定時間を過ぎてしまった記事は表示されないようです。

[shobu]

三度スミマセン。

以下の方法で解決可能かどうか、副作用やマイナス点が無いか、ご意見を戴けるでしょうか？

・sitePublishing.idx.php の書き換え前、常にfrontでの公開処理同様の処理を呼び出す（これが何処か、今は把握していませんが）。
　sitePublishing.idx.php 書き換え前の時間を過ぎていれば、そこまでに処理しないといけないドキュメントは全て処理されるはず。その後、その時点より後の直近の処理時間を書き込む。高負荷時のタイムラグを考慮すると、書き換え直前に now() で基準を決定するよりは、書き換え前の時間を保持して、それより後の直近時間を探すようにした方が良いと思われる。

[shobu]

連投スミマセン。

とりあえず cache_sync.class.processor.php 内の直近の公開非公開処理時間取得処理の直前、

/* PUBLISH TIME FILE */

というコメントの後ろに

$modx->checkPublishStatus();

を入れてみました。checkPublishStatus() は documentParser内にある公開、非公開を処理する関数です。
一応、これでテストすると、記事の保存時点でそこまでに公開、非公開処理されていなければならないものが全て処理されるようなので、この問題は一応収束しますが、これが他の条件や動作も含めて考えた時に、良い処理なのかどうか、判断は出来ませんでした。

よりベターな方法があれば教えて頂ければ幸いです。

[yama]

なるほど、一番よさそうなタイミングのように思えます。もしよければプルリクエストいただければマージさせていただきます。

[shobu]

なるほど、一番よさそうなタイミングのように思えます。もしよければプルリクエストいただければマージさせていただきます。

お返事ありがとうございます。

テストも不十分なので、このまま採用となってしまうと次のリリース時、エンバグした状態にしてしまうかなぁという心配をしていて、yamaさん含め、お詳しい方々に、その辺の評価もお聞きしたかったのです。

管理画面でのパフォーマンスが多少落ちるぐらいでしょうか。見逃している致命的な問題はないもんでしょうかね。。。
全体のイベントのタイミングとか処理について、特に詳しい訳ではないので心配なのですが。

とりえず、手元のテスト環境では目立った問題は無いように見えるのですが、高負荷時にも安定した動作ということを考えると、

$modx->checkPublishStatus(); の使用が問題ないという前提で

1. $modx->checkPublishStatus(); が完了した時点での時刻を保持
　　この行の直後で時刻取得しても良いですが、より精度を求めると checkPublishStatus() 内で公開・非公開処理が完了した瞬間の時間、または公開・非公開処理をした記事の中で現時点に最も近い公開・非公開時間が返値されて保持できるといいでしょうね。

2. SQLでの current_Timeを上記で保持した時間にする。こうすることで checkPublishStatus() と次回の処理時間を得る処理までの間に遅延が出ても、この遅延の間に含まれる公開・非公開時間があっても大丈夫。

という流れかなぁ。
とりあえず $modx->checkPublishStatus(); 使用可否、副作用の有無が確認出来れば、 $modx->checkPublishStatus(); を追加して、今よりは問題発生の確率を大幅に減らせますね。

[shobu]

こんにちは。

なじみ深い古いバージョン（1.0.6J-r4）の document.parser.class.inc.php 内の checkPublishStatus() を眺めてみましたが、関数の後半で sitePublishing.idx.php の更新を行なっていました。

よって、 cache_sync側で checkPublishStatus() を呼び出すならば、cache_syncにある sitePublishing.idx.php の更新処理はバッサリ必要なくなります。

しかし、落とし穴があり、 checkPublishStatus() の中に

コード: 全て選択

if ($cacheRefreshTime <= $timeNow &&  $cacheRefreshTime != 0) {

という条件式があるため、初めての時間設定で $cacheRefreshTime=0 となるときや、既に現時点より先の時間設定を行なって $cacheRefreshTime > $timeNow となるケースでは checkPublishStatus() のメイン処理は走ることはありません。

そこで、 checkPublishStatus()を少し書き換え引数を取れるようにします。

コード: 全て選択

function checkPublishStatus($backend = false) {

次に先の条件式を

コード: 全て選択

if (($cacheRefreshTime <= $timeNow &&  $cacheRefreshTime != 0) || $backend) {

とします。
この状態で cache_sync 側での呼び出しを

コード: 全て選択

$modx->checkPublishStatus(true);

とすれば、管理画面での保存時に常に checkPublishStatus() 内の公開・非公開処理と sitePublishing.idx.php の更新処理が走るので問題が解決されそうです。
（cache_sync 側での sitePublishing.idx.php の更新処理は全てコメントアウトすることになります）

checkPublishStatus() では、関数に入ってすぐに、その時点の時間を保持して、公開・非公開処理の基準から sitePublishing.idx.php への次の処理時間書込処理まで、一貫して最初に取得した時間を使用しているので、途中でタイムラグが出たとしても、確実にある一点の時間を境にして公開非公開と次の処理時間を決定出来ると思います。

よって、最新バージョンと同じ処理になる前までは上記の流れで良さそうに思えますがどうでしょうか。

最新バージョンの方は別途検討が必要そうです。。。
最新版だと処理が大幅に変わっているようで、 checkPublishStatus() で sitePublishing.idx.php の更新までは行なっていないのですが、どこでこの処理をしているのかが分かりませんでした。
とりあえず、 cache_sync.class.processor.php の $current_time = time(); の直後で checkPublishStatus() を呼び出すと、公開・非公開の処理をした少し前の時間を基準に次の処理をするので、条件とタイミングによっては無駄が出る可能性はありますが、処理は抜け落ちる記事の発生はなくなると思います。

[yama]

ポイントとなる箇所は２ヶ所ある感じですが、

コード: 全て選択

$result = $modx->db->select('MIN(pub_date) AS minpub','[+prefix+]site_content', "{$current_time} < pub_date");

ここはとりあえず修正する必要がありそうですね。条件としては、publishedがゼロのリソース全てを対象に、pub_dateの値が最も小さいもの(ただしゼロは除く)を抽出するのがよいのかな？と思います。

[shobu]

ポイントとなる箇所は２ヶ所ある感じですが、

コード: 全て選択

$result = $modx->db->select('MIN(pub_date) AS minpub','[+prefix+]site_content', "{$current_time} < pub_date");

ここはとりあえず修正する必要がありそうですね。条件としては、publishedがゼロのリソース全てを対象に、pub_dateの値が最も小さいもの(ただしゼロは除く)を抽出するのがよいのかな？と思います。

上記の様に考えて SQL直接回して色々検討したのですが、少々難しいようです。
上記の条件だと過去の記事で、非公開時間設定がされて非公開になっている記事も対象になってしまいます。こうなると、現時点よりもかなり前の時間が取得されてしまう可能性があり、単純なロジックでは駄目そうなのです。
それで一連の投稿のように記事保存時にその時点で処理されるべきものを処理してしまってから、その次の時間を決めるのが良かろう、、、という考えになっています。

[yama]

それで一連の投稿のように記事保存時にその時点で処理されるべきものを処理してしまってから、その次の時間を決めるのが良かろう、、、という考えになっています。

なるほど、そういうことなんですね。その考え方だとシンプルで副作用もなさそうに思います。($recentEventみたいなのを追加？)
最新版ではキャッシュの更新は$modx->clearCache()ひとつにまとめていて、sitePublishing.idx.phpの更新もそこで行ないます。（実際はcacheクラスを呼び出して行なうため、少し見通しが悪いですが）

[shobu]

新しいバージョンも検討してみました。

1.0.6以降は少し複雑ですね。前述の流れでは対処出来ません。

まず、管理画面での記事保存時、 cache_sync.class.processor.php 内の emptyCache() 内から publish_time_file() が呼び出されることで sitePublishing.idx.php の更新がなされます。

ここで先ほどのように publish_time_file() のタイミングで公開・非公開処理を走らせようとして checkPublishStatus() を呼び出してしまうと、その先で cache_sync.class.processor.php 内の emptyCache() を呼び出してしまいループしてしまうようです。

通常はフロント側のアクセス時に checkPublishStatus() が呼び出されるわけですが、この処理のメイン部分が走ると、その中で呼び出している clearCache() 関数内で cache_sync.class.processor.php をincludeして cache_sync内の emptyCache() を呼び出し、結果 publish_time_file() が走る形になってループするようです。

checkPublishStatus() → clearCache() → emptyCache() → publish_time_file() → 改造して checkPublishStatus() を呼び出すとループ

checkPublishStatus() では emptyCache() 呼び出し前に公開・非公開を行ない、その後に emptyCache() を経由して publish_time_file() を呼び出し。
publish_time_file() の中で次回更新時間を決定してsitePublishing.idx.php を更新していました。

※少し前のバージョンは checkPublishStatus() 内の clearCache() 呼び出し以降、もう一度次回更新時間を決定してsitePublishing.idx.php を更新する流れでしたが、最新版では無駄なためか削除されていますね。

上記の流れをもう少し整理して

1. checkPublishStatus() 内の公開・非公開処理を publish_time_file() 内に移動。これで publish_time_file() の先頭で現在時刻を取得し、一貫してこの時刻で公開・非公開処理と次の処理時間の決定を行なうようにしてタイムラグによる欠落を防止。
2. フロントからのアクセスによる公開・非公開処理時は checkPublishStatus() から最終的に publish_time_file() が呼び出されるので実行される。
3. 管理画面での記事保存時はオリジナルのまま emptyCache() → publish_time_file() とするが、1．の処置により公開・非公開処理後にsitePublishing.idx.php が更新される。

上記の変更で気になっているのですが判断が出来ていない点は

★$this->setChunkCache(); のタイミング
checkPublishStatus()で

コード: 全て選択

unset($this->chunkCache);
$this->setChunkCache();
// clear the cache
$this->clearCache();

と $this->setChunkCache(); を呼び出す処理が clearCache の前にある。setChunkCache() は公開・非公開処理後に呼び出すべき？

★emptyCache内のbuildCache()のタイミング
emptyCache() 内で publish_time_file() を呼び出す前に以下の処理が入る。

コード: 全て選択

if(strpos($this->target,'pagecache')!==false) $result = $this->emptyPageCache('pageCache');
if(strpos($this->target,'sitecache')!==false) $this->buildCache($modx);
$this->publish_time_file($modx);

emptyPageCache() と buildCache() も公開・非公開処理の後に走るべき？

emptyCache() 内で以下のような流れにする必要があるでしょうか…

コード: 全て選択

$this->publish_time_file($modx);
if(strpos($this->target,'pagecache')!==false) $result = $this->emptyPageCache('pageCache');
if(strpos($this->target,'sitecache')!==false) $this->buildCache($modx);
$modx->setChunkCache();

[shobu]

それで一連の投稿のように記事保存時にその時点で処理されるべきものを処理してしまってから、その次の時間を決めるのが良かろう、、、という考えになっています。

なるほど、そういうことなんですね。その考え方だとシンプルで副作用もなさそうに思います。($recentEventみたいなのを追加？)
最新版ではキャッシュの更新は$modx->clearCache()ひとつにまとめていて、sitePublishing.idx.phpの更新もそこで行ないます。（実際はcacheクラスを呼び出して行なうため、少し見通しが悪いですが）

お返事ありがとうございます。行き違いになりましたが、最新版の clearCache() の件も追っていました。１つ前のコメントをご確認戴ければ。
処理の途中に挟まるキャッシュの処理等への妥当性が判断出来ていませんが、どうでしょうか？

[yama]

cache_sync.class.processor.phpの中から$modx->checkPublishStatus()を呼び出すのがいいかな？と思いましたが、やめといたほうがよさそうですね。ループしないように工夫することはできても、ただでさえ紛らわしい関数名が多いので読みづらくなりそうです。shobuさんの解析内容をまだよく理解できてないかもですが、同じような処理でもコピーして持ってくるか、別の関数として作って両方からそれぞれ呼び出すのがよいかも？
こちらでもじっくり長考してみます。

[shobu]

お返事ありがとうございます。

頭がこんがらがっていますが、ザックリ考えると、documentParserの方は触らずに、とりあえず publish_time_file() の中でも公開・非公開の処理をしてから次回処理時間を決定するようにしちゃえばいいですかね。
そうなると公開・非公開の処理は別関数に…となりそうですが、公開・非公開処理での基準時間と、次回処理時間の基準時間を一致させることが肝要だと思います。
（それぞれズレがあると、可能性は小さいですがズレの狭間に公開・非公開時間が設定された記事があると欠落しますので）
別関数にするなら基準時間を引き継げるようにした方が良いかと思います。

ご検討よろしくお願いいたします。

[yama]

基準時間というのは、現在のコードではtime()になってる部分ですよね。
これを$recentRefreshTimeという感じの変数名で$cacheRefreshTimeといっしょにsitePublishing.idx.phpに都度記録するようにすると使いやすいかな？という気がします。
($lastRefreshedTime・$nextRefreshTimeとしたいところだけど、変えるとたぶんまずいですよね・・)

[shobu]

基準時間というのは、現在のコードではtime()になってる部分ですよね。
これを$recentRefreshTimeという感じの変数名で$cacheRefreshTimeといっしょにsitePublishing.idx.phpに都度記録するようにすると使いやすいかな？という気がします。
($lastRefreshedTime・$nextRefreshTimeとしたいところだけど、変えるとたぶんまずいですよね・・)

$cacheRefreshTime の表記はあちらこちらにあるので触らぬ方が良いように思います。
また、基準時間と言っているのは time() の部分のことです。
公開・非公開の処理と、sitePublishing.idx.phpの更新が別の処理になっている場合に、それぞれの処理の中で time() で基準時間を決めると、高負荷時などで差が出てしまう可能性があり、その差の間に公開・非公開時間が設定された記事が処理外になってしまう可能性があるはずです。
これを防ぐ上で、公開・非公開処理から sitePublishing.idx.php の更新までが一連の処理であり、冒頭で決定した基準時間で全て処理されるのが望ましいと思います。

とりあえずは
・記事保存時とフロントでのページ表示時に公開・非公開とsitePublishing.idx.phpの更新を一括で行なう
・基準時間を１つにする
さえ守れれば他の処理で $lastRefreshedTime に相当する上記の基準時間は必要無いように思います。
例えば、高負荷時、基準時間を決定してから一連の処理が経過するまでに極端ですが60秒かかったとすると、次に処理の有無を判断するときにはその時点よりも60秒以上前だということで、改めて処理が走ることになり、その60秒の間に処理すべきだった記事も処理の対象になると思います。
よって常に処理時点と$cacheRefreshTimeの比較で問題ないので、$lastRefreshedTime・$nextRefreshTime を用意する必要性は、この件に関してはないかなぁ、という所です。

[yama]

なるほど。$_SERVER['REQUEST_TIME']みたいなのを使うという感じでしょうか？(さっきググったら出てきました)

[shobu]

なるほど。$_SERVER['REQUEST_TIME']みたいなのを使うという感じでしょうか？(さっきググったら出てきました)

time()で十分か、、、と思いましたが、 $_SERVER['REQUEST_TIME'] だと処理のどこから呼び出しても、ページ表示時、保存時の瞬間の時間が返るので基準時間の整合性は取れますね。 $_SERVER['REQUEST_TIME'] に環境による違いなどが無ければ、こちらの方が良さそうですね。
$_SERVER['REQUEST_TIME'] はtime() より速いなんて言う tips もあったようなw

ソース眺めていてチョット気になったのですが、file_put_contents で sitePublishing.idx.php を書き込んでいますが、ロック処理、問題ないんでしたっけ？
古いphpのバージョンだとロック指定してもロックされないバグもあった気がするのと、LOCK_EX のフラグ無しでロックが保証されるのかチョット気になります。
今まで sitePublishing.idx.php 破損した経験は無いですが…。

[yama]

https://github.com/modxcms-jp/evolution ... 0437e20c26
$_SERVER['REQUEST_TIME']の件だけとりあえず対応しておきました。

file_put_contentsのロックの問題は最近知りましたが、いずれ対応できればしたいと思います。

[shobu]

こんにちは。
ソース見ました。

とりあえず、フロントでのページ表示の際、公開非公開処理～sitePublishing.idx.php更新の間のタイムラグの間に、運の悪い記事の時間制定が入ってしまうケースは無くなりますね（1秒ごとに次々に公開、非公開なんてしてると状況によっては可能性あると思います）。

主問題の非公開のまま設定時間を過ぎてしまった記事の件を解決するには、新バージョンにおいてはキャッシュビルドのタイミング他について、よく調査する必要がありそうですね。
古いバージョンでの対応は先日の形で問題なさそうです。それなりの規模のサイトでバックエンド・フロントエンドのタイマー処理含め、安定稼働しています。
処理タイミングの点について確認が取れれば前のコメントのように処理の順番を入れ替えて対応できそうですが…。手元の環境で少しテスト＆検討してみることにします。

file_put_contents の件、grepでソース検索すると、使用している殆どの箇所で LOCK_EX が付けてあり、時々抜けがあるという感じでした。

actions/bkmanager.static.php:

コード: 全て選択

90: file_put_contents("{$modx->config['snapshot_path']}.htaccess",$htaccess);
647: file_put_contents($path,$dumpstring,FILE_APPEND);

actions/files.dynamic.php:

コード: 全て選択

329: $rs = file_put_contents("{$startpath}/{$filename}",'');
649: file_put_contents($complete_name, zip_entry_read($zip_entry, zip_entry_filesize($zip_entry)));

actions/files.dynamic.php

コード: 全て選択

773: if (file_put_contents($filename, $content) === FALSE)

processors/cache_sync.class.processor.php:

コード: 全て選択

302: file_put_contents($this->cachePath . '.htaccess', "order deny,allow\ndeny from all\n");

processors/save_role.processor.php:

コード: 全て選択

61: file_put_contents($cache_path, serialize($role));

processors/save_settings.processor.php:

コード: 全て選択

28: if(!@file_put_contents($htaccess,$_))

includes/extenders/export.class.inc.php:

コード: 全て選択

125: $result = file_put_contents($filepath,$src);

今、問題にしている部分以外でもキャッシュ関連へ書き込みしている箇所があるようで、一応ロックした方が良いでしょうね。
吟味していませんが、全箇所に LOCK_EX 入れても著しい速度低下などはなさそうですし、キャッシュ関連をロックしていない状態は別の方向から指定時間に記事が見えないなどの不具合の原因になってきそうです。

引き続きよろしくお願いいたします。