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.

Sign up for GitHub

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

Jump to bottom

サクラエディタの起動テストを追加する #1385

Merged
merged 7 commits into from
Sep 21, 2020
Merged

サクラエディタの起動テストを追加する #1385

Changes from 1 commit
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
9da897d
サクラエディタの起動テストを追加する
berryzplus Aug 30, 2020
646fe63
Revert "サクラエディタの起動テストを追加する"
berryzplus Sep 15, 2020
977a210
サクラエディタの起動テストを追加する
berryzplus Sep 15, 2020
6985c78
tests1.exeをサクラエディタとして起動できるようにする
berryzplus Sep 16, 2020
0de5be5
起動テストを外部プロセスで実行するように変更する
berryzplus Sep 16, 2020
22dc1db
関係ないファイルの変更をrevert
berryzplus Sep 16, 2020
dba3441
起動したコントロールプロセスを待機する処理を修正する
berryzplus Sep 17, 2020
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
33 changes: 3 additions & 30 deletions sakura_core/_main/CProcessFactory.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -230,20 +230,6 @@ bool CProcessFactory::StartControlProcess()
return false;
}

// 起動したプロセスが完全に立ち上がるまでちょっと待つ．
//
// Note: この待ちにより、ここで起動したコントロールプロセスが競争に生き残れなかった場合でも、
// 唯一生き残ったコントロールプロセスが多重起動防止用ミューテックスを作成しているはず。
//
int nResult;
nResult = ::WaitForInputIdle( p.hProcess, 10000 ); // 最大10秒間待つ
Copy link
Contributor Author

Choose a reason for hiding this comment

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

問題点の1つ目です。
WaitForInputIdleはプロセス同期を簡易化する目的で用意された便利関数なので機能に制限があります。

制限

  • 対象が GUIプロセス でなければ利用できない。
  • 対象が マルチスレッド である場合の挙動が保証されない。
  • アイドル検出のタイミングを制御できない。

より高度な同期オブジェクトを使える場合にはそちらを使うべきで、ここのコードは既に同期オブジェクト（イベントオブジェクト）を利用しています。

Copy link
Contributor

Choose a reason for hiding this comment

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

コントロールプロセスの初期化完了を後の処理でイベントを使って確認しているのでこのチェックは不要だという事ですね。

このチェックが有っても害は無さそうだけど、テストする都合上削りたい的な事なんじゃないかと思います。

WaitForInputIdle ってプロセスがユーザー入力を受け付けることができるアイドル状態になっているかを確認するAPIのようですが、コントロールプロセスが GUI を持つかというとタスクトレイのウィンドウを作ってメッセージループを回すので該当すると思います。

さてこのチェックが無くても本当に問題が無いのか？ですが、まぁ多分大丈夫なんじゃないでしょうか…。コントロールプロセスとエディタプロセス間でユーザー入力による待ち受けとかは無いので。

if( 0 != nResult ){
ErrorMessage( NULL, L"\'%ls\'\nコントロールプロセスの起動に失敗しました。", szEXE );
::CloseHandle( p.hThread );
::CloseHandle( p.hProcess );
return false;
}

::CloseHandle( p.hThread );
::CloseHandle( p.hProcess );

Expand All @@ -264,27 +250,14 @@ bool CProcessFactory::WaitForInitializedControlProcess()
// Note: コントロールプロセス側は多重起動防止用ミューテックスを ::CreateMutex() で
// 作成するよりも先に初期化完了イベントを ::CreateEvent() で作成する。
//
if( !IsExistControlProcess() ){
// コントロールプロセスが多重起動防止用のミューテックス作成前に異常終了した場合など
return false;
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

問題点の2つ目です。
このコードは「コントロールプロセスを起動した」ときにだけ呼ばれるコードです。
IsEistControlProcessで確認するのは「コントロールプロセスが既に起動されているかどうか」なので、ちょっと違うチェックをしています。エディタプロセス的にはコントロールプロセスが排他制御ミューテックスを作成しているかどうかに気にしなくてよいことです。（後続をOpenEventするためだけに必要。）


const auto pszProfileName = CCommandLine::getInstance()->GetProfileName();
std::wstring strInitEvent = GSTR_EVENT_SAKURA_CP_INITIALIZED;
strInitEvent += pszProfileName;
HANDLE hEvent;
hEvent = ::OpenEvent( EVENT_ALL_ACCESS, FALSE, strInitEvent.c_str() );
hEvent = ::CreateEventW( NULL, TRUE, FALSE, strInitEvent.c_str() );
Copy link
Contributor Author

Choose a reason for hiding this comment

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

これが修正のメインです。問題点の3つ目です。
動いているので「不具合とまでは言えない」ですが、やり方が間違っています。

このやり方を成立させるには、このコードが実行される前に、起動されたプロセスがCreateEventを完了している必要があります。イベント発動を待機するにあたり、イベント発動するスレッドがイベントを開いていることは必要条件ではないので、ノーチェックで CreateEvent してしまえばよいです。

Copy link
Contributor

Choose a reason for hiding this comment

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

これは問題無さそうですね。

if( NULL == hEvent ){
// 動作中のコントロールプロセスを旧バージョンとみなし、イベントを待たずに処理を進める
//
// Note: Ver1.5.9.91以前のバージョンは初期化完了イベントを作らない。
// このため、コントロールプロセスが常駐していないときに複数ウィンドウをほぼ
// 同時に起動すると、競争に生き残れなかったコントロールプロセスの親プロセスや、
// 僅かに出遅れてコントロールプロセスを作成しなかったプロセスでも、
// コントロールプロセスの初期化処理を追い越してしまい、異常終了したり、
// 「タブバーが表示されない」のような問題が発生していた。
//
return true;
beru marked this conversation as resolved.
Show resolved Hide resolved
TopErrorMessage( NULL, L"エディタまたはシステムがビジー状態です。\nしばらく待って開きなおしてください。" );
return false;
}
DWORD dwRet;
dwRet = ::WaitForSingleObject( hEvent, 10000 ); // 最大10秒間待つ
Expand Down