ワーカースレッドを起動するとAccess Violation

[戻る]
たちき 2006/07/14(金) 17:40:54
何度も質問ばかりですみません。

funcという関数をワーカースレッドを用いて処理しようとしています。
AfxBeginThreadでfuncを呼ぶプログラムを作成する前に、
テストの意味をかねて直接funcを実行してみました。
その場合は、マルチスレッドじゃないのでコマンドは受け付けませんが、
期待通りの処理が行われました。
なのでAfxBeginThreadから呼んでも大丈夫だろうと思ってコーディングすると、
func関数の内容は変更していないのにAccess Violationエラーが発生してしまいます。
スレッド内でファイルの開閉を行っているのですが、そこでエラーになっているみたいです。

エラーの原因等が全く検討つきません。
なにとぞご教授ください。お願いいたします。

以下にプログラムの大まかな流れを表示します。

---------------------------
void CtestDlg::OnButton()
{
  ...
  XXX x(path); // コンストラクタでpathで指定されたファイルをオープン
  AfxBeginThread(Thread , &x); // スレッド開始
  // Thread(&x); // ←この記述なら正常に動作する
  ...
}
--------------------------
class XXX
{
public:
  CStdioFile cf;
  XXX(CString path)
  {
    cf.Open(path,CFile::modeRead);
  }
  ...
};
--------------------------
UINT Thread(LPVOID pParam)
{
  XXX* x = (XXX*)pParam;
  ...
  x->cf.Close(); // ←ここでAccess Violationエラーになってるっぽい
  return 0;
}

たちき 2006/07/14(金) 17:41:25
すみません、環境はWindows2000Pro、VC++6.0(MFC)です。
よろしくお願いいたします。

foo 2006/07/14(金) 17:56:17
xの寿命の問題です。

シングルスレッドの場合、xが生成されThread()が呼ばれ処理が完了した後にxが破棄され
ます。

マルチスレッドの場合、xが生成され別スレッドが起動し、Thread()が終了したかどうか
に関係なく、xが破棄されます。
xが破棄された後、別スレッドでxが参照(x->cf.Close(); )された場合、当然アクセス
違反で終了します。

Ban 2006/07/14(金) 18:00:01
上記の記述だと、xはOnButtonのローカル変数なので、OnButtonを抜けた瞬間に
無効になります。このため、非同期実行する実際のThreadが動く時点で、
xのポインタの参照先が無効になっており、不正になります。


# ためしに、OnButtonのxをstaticとかにしたら動きませんか?
# (これはあくまで問題きりわけのためのテスト用です。本来的な対応にはお勧めできま
せん)

foo 2006/07/14(金) 18:00:37
この問題を回避するには、xをメンバ変数にするなどしてThread()が終了するまで、xが破
棄されないようにすれば良いです。

Ban 2006/07/14(金) 18:03:07
一般的には、foo さんが書かれている対応が本来的に好ましい可能性が高いです。

たちき 2006/07/14(金) 18:09:10
[[解決]]
foo様、Ban様、返答ありがとうございます。

まずは、Ban様の指摘に従いOnButton内のクラスの宣言をstaticで行うと、
エラーになることなく正常に動作いたしました。
ちゃんと動作しているのでこのままにしたい気持ちが大きいのですが、
この対応はあまりよくない対応なのでしょうか?

また、正しい解釈かどうかわかりませんが、
OnButton関数でxをstatic無しで宣言した場合、
OnButton関数が終了してしまうとxが破棄されてしまう。
つまり、OnButton関数がThread関数よりも早く終わってしまうと、
Thread関数でxを読もうとしてもそれはもう寿命が尽きているのでエラーになる、
ということでしょうか。

なにはともあれ、かなり勉強になりました。
ありがとうございました。

Blue 2006/07/14(金) 18:36:24
メインのスレッド(ダイアログ)がサブスレッド(XXX)よりも
はやくなくなる(たとえば、×ボタンで終わらされる)のを考慮
(サブスレッドではメインスレッドのメンバ変数等の情報を使っているので)して、
AfxBeginThreadの戻り値を管理したほうがよいかも。
(OnDestoryで正常にサブスレッドが終わるようにする)


この考え方間違っていたら、ご指摘お願いします。

Blue 2006/07/14(金) 18:54:53
あーなんか、プロセスというわけじゃな
いから、メインがおわれば勝手にサブも
強制的に終わる気がしてきた。

2006/07/14(金) 18:58:54
インターフェースの変更が許されるのならば
スレッド内でクラスXXXを生成、破棄したほうが自然です。

複数のスレッドでクラスを参照するときはスレッドセーフかどうかの考慮がいる?ので
pathもCStringではなくcharのが個人的にはオススメです。


void CtestDlg::OnButton()
{
  ...
  char szPath[]; <-こいつにpathをコピー
  ...
  AfxBeginThread(Thread , szPath); // スレッド開始
   ...
}

--------------------------
UINT Thread(LPVOID pParam)
{

  XXX x pParam;
  ...
  x->cf.Close(); // ←ここでAccess Violationエラーになってるっぽい
  return 0;
}

2006/07/14(金) 19:02:19
編集中に誤って送信してしまいました。
上のThread()はまちがいです。
ごめんなさい。

--------------------------
UINT Thread(LPVOID pParam)
{
  char* path = (char*)pParam;
  XXX(path);
  ...
  x.cf.Close(); // ←ここでAccess Violationエラーになってるっぽい
  return 0;
}

ITO 2006/07/14(金) 20:21:41
> あーなんか、プロセスというわけじゃな
> いから、メインがおわれば勝手にサブも
> 強制的に終わる気がしてきた。
強制的に終わらせるのでメモリーリークを引き起こします。

なので、blueさんと同意見です。
>(OnDestoryで正常にサブスレッドが終わるようにする)

グローバルの変数、もしくは構造体かクラスを作ってスレッド上で
動かすてもあると思います。
競合に気をつけなければいけませんが........

Ban 2006/07/15(土) 02:04:26
確認ですが、本当に「ちゃんと」動いてますか。
どちらが先に終るかわからなかったので、かならずxが残るように、
そこだけを見て、xの寿命を延ばしてみただけなんですけど。

XXXのコンカレンシ次第で別のバクがでるはずで(憶測ですが確率はかなり高そう)、
私にはXXXの正体もわかりませんし、意味的な妥当性やシステムの整合性は未検証ですよ。
これらを正しく検討した上での判断なら大丈夫なことも(適することさえ)ありますが、
「とりあえず動いた」だけだと、頻度/確率的にはかなり部が悪い賭けじゃないかと思いますが。

staticはインスタンスが一つしか持てませんので、多分目的にはあわないんじゃないですか?
であれば、バグ持ちのやっつけ対応はやめておくが吉と思います。

元のコードの単純なstaticだと、(xの実処理にもよりますが)ボタンの連打
(スレッドが同時に複数走る状態)でおかしくなるのでは?
排他制御もなさそうなので、それで多分レースコンディションになります。

対策として、メンバ変数+排他がいいか、動的生成がいいかは、スレッドの実際の用途によるので、
今の情報ではわかりかねます。


なお、メインが終ればサブも終ります。
ただし、終了と参照がレースコンディションになって危険なので、
待ち合わせはしたほうがいいと思います。
この場合、サブスレッドが長い(or終らない)処理だと
終了時にメインが固まるので、
スレッドに中断機構を推奨します。

また、自プロセスのハンドルやメモリはOSにより解放されますのでリークはケアされます。
とはいえ外部リソースがあれば漏れますので、結局注意は必要ですが。

ITO 2006/07/15(土) 10:08:40
> XXXのコンカレンシ次第で別のバクがでるはずで(憶測ですが確率はかなり高そう)、
> 私にはXXXの正体もわかりませんし、意味的な妥当性やシステムの整合性は
> 未検証ですよ。

危険ですね。
僕は、XXXは「CMainFrame」等のメインスレッドにします。
もちろん、スレッドもここで作成する。
XXX *x;
をグローバルで作成して、

xxx()
{
   x = this;
}

UINT Thread(LPVOID pParam)
{
  XXX* x = (XXX*)pParam; ---->削除
  ...
 pParamは、メインスレッドとのパラメ-タ設定のみ使用する。

  x->cf.Close(); // ←ここでAccess Violationエラーになってるっぽい
  ウインドウ操作に関する関数は「x->」から起動しないこと。
  どこかで HWND=NULLの例外が起こる。
 return 0;
}

>また、自プロセスのハンドルやメモリはOSにより解放されますのでリークはケアされま
す。
そうなんですか、デバックモードで見ると必ずメモリーリークのダンプが出たので危険
だと思っていますが.......

ITO 2006/07/15(土) 11:22:38
修正します(^^;


誤
> xxx()
> {
>    x = this;
> }

正
xxx::xxx()
{
    x = this;
}

Ban 2006/07/15(土) 11:33:47
もちろん、リークはバグですし直すべきです。

ITRON等のようにケアしてくれないOSもありますし、
終らない限りだだ漏れですし。

ただWindowsのような昨今の高級OSでは、
「プロセス自体が終わる」のなら、
その時にOSが、そんな駄目プロセスのせいでOS自身や他のプロセスまで
おかしくならないように、わりあててあげてた分を全部取りあげます。
メモリやハンドルはOS管理なので貸してた量はわかりますし。

ただし使い方には関知してませんから無視して破棄されます。
外部リソースもどう使ったか関知してませんので同様です。
これらの終了処理は無視されると一般に危険です。
#自発的にきちんと返さないと怖いおにいさんの取り立てが来る、と。

ITO 2006/07/15(土) 13:22:30
> おかしくならないように、わりあててあげてた分を全部取りあげます。
> メモリやハンドルはOS管理なので貸してた量はわかりますし。
なるほど、デバック時にでるメモリーリークのダンプ表示は、OSで処理されます
という警告でもあるのですね。
>#自発的にきちんと返さないと怖いおにいさんの取り立てが来る、と。
そうですね、正常終了に越したことないですね。

Ban 2006/07/15(土) 21:50:43
その警告自体はVCのデバッグ版ライブラリが出してますから(VCのCRT付加機能)、
まだブロセス内の話で、また別ちょっと違った立場だと思いますが、
(怖いおにいさんが来る前に、心配してくれる親?)
その後OSに(無言で)捨てられるので、まぁ見た目の結果は大差ないですね。

PATIO 2006/07/18(火) 15:49:19
スレッドを使う上でメインスレッドからワーカースレッド等のサブスレッドを
安全に終わらせる手段まで考慮する必要があるという事を認識してスレッドを
使用していない方が結構いらっしゃるみたいです。
結果的にOSが後始末をしてくれるとは言っても、本来はプログラムの開始から
終了までの処理はプログラム側で責任を持って行うべき物ですし、
OSの後始末をする機能は、最悪の事態でもシステム全体への影響を最小限にする為の
物と考えるべきだと思います。
ワーカースレッドが動作中にメインスレッドを終了させるような場合に
安全にワーカースレッドを終了させる為の仕組み等々は設計の時点でちゃんと
考えておくべきだと私は思います。

PATIO 2006/07/18(火) 15:51:09
うわー、わかりにくいので修正。

> スレッドを使う上でメインスレッドからワーカースレッド等のサブスレッドを
> 安全に終わらせる手段まで考慮する必要があるという事を認識してスレッドを
> 使用していない方が結構いらっしゃるみたいです。

スレッドを使う上でメインスレッドからワーカースレッド等のサブスレッドを
安全に終わらせる手段まで考慮する必要があるという事を認識しないで
スレッドを使用している人が結構いらっしゃるみたいです。

Ban 2006/07/18(火) 16:15:40
>PATIOさん

御意。
外部リソースの面倒とか見てくれませんし。

Blue 2006/07/18(火) 16:26:39
> Banさん、ITOさん、PATIOさん
いろいろ勉強になりました。

http://rararahp.cool.ne.jp/cgi-bin/lng/vc/vclng.cgi?print+200607/06070042.txt
のすれで、

Blue 2006/07/14(金) 17:53:04
> CEditのポインタでなく、エディットボックスのウィンドウハンドルでSendMessage
> する方法でやったほうがよいでしょう。
> (CEditのポインタだと、ダイアログスレッドが終わるとアクセスできなくなる。
> スレッドになる以上こういう状態はありうる)
と書いたんですが、親が子をきちんと管理すれば、このような考えはしなくてもよかっ
たのですね。
(というか、こういう状態にはなりえないということですよね?)

ITO 2006/07/19(水) 10:37:22
>と書いたんですが、親が子をきちんと管理すれば、このような考えはしなくてもよかっ
>たのですね。
>(というか、こういう状態にはなりえないということですよね?)
親のthisポインタを取得してそこからCEditのポインターを得れば大丈夫だと思うの
ですが、CEditのポインターだと心配が残りますね。

PATIO 2006/07/19(水) 12:01:29
基本的には互いにアクセスする必要がある時点で全てのスレッドの寿命は
きちんと管理されている必要があります。
メインスレッドはサブスレッドの生き死にをきちんと把握しているべきですし、
サブスレッドからメインスレッドに何らかの通知を行う場合は、
サブスレッドが生きている間はメインスレッドがきちんと生きている保障を
するべきです。
あと、スレッドを跨いだCWndオブジェクトのやり取りはMicrosoftが公式に
してはいけないと書いていますからするべきではありません。
基本的には親スレッドのウインドウハンドル宛にユーザー定義メッセージを
送ってメインスレッド側で画面側の更新を行うとかするべきでしょう。
基本的にウインドウメッセージを使ったやり取りにした方がスレッド間も
疎結合になるので良いと思います。

PATIO 2006/07/19(水) 12:07:02
補足。
> あと、スレッドを跨いだCWndオブジェクトのやり取りはMicrosoftが公式に
> してはいけないと書いていますからするべきではありません。

現在のMFCの実装をソースで確認して大丈夫だから使えるとおっしゃる方も
いますが、基本的には公式にしてはいけないと書いている事をすべきではありません。
これは、Microsoftが今後実装に変更を加えて出来ないようにしても
公式見解と矛盾が無いのでわれわれから文句を言う余地が無いという事を含んでいます。
今、使用できる実装になっていても今後その実装が保障されていないのであれば
使用するべきではありませんし、他の人に進めるべきでもないと私は考えます。

PATIO 2006/07/19(水) 12:08:41
がーっ、誤字

誤)
> 使用するべきではありませんし、他の人に進めるべきでもないと私は考えます。

正)
使用するべきではありませんし、他の人に勧めるべきでもないと私は考えます。

ITO 2006/07/19(水) 17:08:25
>あと、スレッドを跨いだCWndオブジェクトのやり取りはMicrosoftが公式に
>してはいけないと書いていますからするべきではありません。
HWND = NULL の例外で何度も悩まされました。

http://rararahp.cool.ne.jp/cgi-bin/lng/vc/vclng.cgi?print+200607/06070042.txt
スレでやろうとしているのが、「SetWindowsText」による値の書換えなので
問題ないと思ったのですが........
もちろん、ウインドウのサイズ変更・色の変更・位置の変更等のウインドウ自身の
操作に関することは、メッセージ等で親ウインドウに知らせて親ウインドウで行わない
といけないと思います。

>今、使用できる実装になっていても今後その実装が保障されていないのであれば
>使用するべきではありませんし、他の人に勧めるべきでもないと私は考えます。
そうですね。今後気をつけます。

PATIO 2006/07/19(水) 17:30:42
> スレでやろうとしているのが、「SetWindowsText」による値の書換えなので
> 問題ないと思ったのですが........
> もちろん、ウインドウのサイズ変更・色の変更・位置の変更等のウインドウ自身の
> 操作に関することは、メッセージ等で親ウインドウに知らせて親ウインドウで行わない
> といけないと思います。

内部的にはSetWindowsTextもウインドウのサイズ変更もたいして変わらないと思いますよ。
SetWindowsTextにしても、内部ではSendMessageで そのコントロールのHWNDにWM_SETTEXTを
飛ばしているだけです。

ある程度以上は個々人のポリシーも出てきてしまうのであんまり押し付けてしまうのも
不味いかもしれませんが、基本的にダイアログ内の操作はダイアログ内で完結した方が
ソースを追いける場合でもわかりやすいと思います。
サブスレッドはダイアログに対して特定の場所を引き渡した文字列で更新する事を
依頼するだけにして実際の更新はダイアログ側が行った方が機能の切り分けとして
すっきりしていると私は考えています。

PATIO 2006/07/19(水) 17:31:20
あがが、
SetWindowsTextではなくてSetWindowTextですね。

ITO 2006/07/19(水) 20:01:31
>あがが、
>SetWindowsTextではなくてSetWindowTextですね。
すみません、こちらが先に間違ってました。

>内部的にはSetWindowsTextもウインドウのサイズ変更もたいして変わらないと思います
>よ。
>SetWindowsTextにしても、内部ではSendMessageで そのコントロールのHWNDに
>WM_SETTEXTを
>飛ばしているだけです。

なるほど、「SendMessage」をつかっているんですね。
直接変えているのかと思っていました。

古いスレッドには追加発言できません。