Нужна критика кода-)

← →
panov © (2004-11-11 18:05) [0]

Опять я с потоками.
Вот реализация класса потока, от которого хочу наследоваться и писать специализированные классы.
Прошу покритиковать тех, у кого есть время и желание. Думаю, что слабых мест достаточно в коде.

unit uThreadClass; interface uses classes,windows; type TThreadClass=class private FHandle,FThreadId: THandle; FEvent: THandle; FFreeOnTerminate: Boolean; FTerminated: Boolean; FSuspended: Boolean; FReturnCode: Integer; procedure FOnStartThread; protected procedure Execute;virtual;abstract; public constructor Create(const CreateSuspended: Boolean); destructor Destroy;override; procedure AfterConstruction; override; procedure FOnEndThread;virtual; procedure Suspend; procedure Resume; procedure WaitFor(Timeout: Cardinal=INFINITE); procedure Terminate; procedure Kill(ExitCode: Cardinal); property FreeOnTerminate: Boolean read FFreeOnTerminate write FFreeOnTerminate; property Terminated: Boolean read FTerminated; property Handle: THandle read FHandle; property ThreadId: THandle read FThreadId; property ReturnCode: Integer read FReturnCode write FReturnCode; end; implementation procedure ThreadProcedure(const Sender: TObject); cdecl; var Ret: Cardinal; begin try if TThreadClass(Sender).FSuspended then TThreadClass(Sender).Suspend; TThreadClass(Sender).FOnStartThread; TThreadClass(Sender).Execute; TThreadClass(Sender).FOnEndThread; Ret := 0; except Ret := 1; end; if TThreadClass(Sender).FreeOnTerminate then TThreadClass(Sender).Free; ExitThread(Ret); end; { TThreadClass } procedure TThreadClass.AfterConstruction; begin inherited; if not FSuspended then begin Resume; WaitForSingleObject(FEvent,INFINITE); end; end; constructor TThreadClass.Create(const CreateSuspended: Boolean); begin inherited Create; FreeOnTerminate := True; FSuspended := CreateSuspended; FEvent := CreateEvent(nil,True,False,nil); FHandle := CreateThread(nil,0,@ThreadProcedure,Self,CREATE_SUSPENDED,FThreadId); AfterConstruction; end; destructor TThreadClass.Destroy; begin CloseHandle(FHandle); CloseHandle(FEvent); inherited; end; procedure TThreadClass.FOnEndThread; begin end; procedure TThreadClass.FOnStartThread; begin if not FSuspended then SetEvent(FEvent); end; procedure TThreadClass.Kill(ExitCode: Cardinal); begin TerminateThread(FHandle,ExitCode); Terminate; Free; end; procedure TThreadClass.Resume; begin ResumeThread(FHandle); end; procedure TThreadClass.Suspend; begin SuspendThread(FHandle); end; procedure TThreadClass.Terminate; begin FTerminated := True; end; procedure TThreadClass.WaitFor(Timeout: Cardinal=INFINITE); begin WaitForSingleObject(FHandle,TimeOut); end; end.

← →
Игорь Шевченко © (2004-11-11 18:11) [1]

> procedure ThreadProcedure(const Sender: TObject); cdecl;

stdcall ?

← →
Игорь Шевченко © (2004-11-11 18:12) [2]

И еще - ты б хоть объяснил, что и зачем, чтобы догадками не заниматься.

← →
Суслик © (2004-11-11 18:13) [3]

1) зачем afterconstruction вызывать?
2) не уверен, что верно, но я бы closehandle делал только если реально соответсвующий handle был инициализирован.
3) зачем в aftercontruction ждать? ведь тогда повиснет вызывающий поток.

← →
DiamondShark © (2004-11-11 18:26) [4]

> CreateThread

BeginThread

← →
DiamondShark © (2004-11-11 18:28) [5]

> procedure [???] ThreadProcedure(const Sender: TObject); cdecl [?!!];

← →
DiamondShark © (2004-11-11 18:30) [6]

Из конструктора вообще никогда не возвращаемся?

← →
DiamondShark © (2004-11-11 18:36) [7]

А можно рассказать, зачем эти странные манипуляции с евентом?

← →
GuAV © (2004-11-11 18:41) [8]

panov © (11.11.04 18:05)
TThreadClass
IMHO следует придерживаться cтандартов на названия. У Вас надо TThreadObj или как-то так, а TXxxxClass - это class of TXxxx

FOnStartThread
Особенно здесь. Это болше похоже на поле события чем на метод.
Вносит путаницу при чтении кода.

ExitThread(Ret);
end;

Зачем так по-странному ? не проще ли вернуть результат ThreadProcedure, сделав её функцией.

procedure ThreadProcedure(const Sender: TObject); stdcall;
Можно сразу написать TThreadClass заместо TObject.
А можно даже объявить stdcall - метод класса (уже без параметра).

← →
Владислав © (2004-11-11 18:57) [9]

Александр, можно поинтересоваться о предназначении этого класса? Чем не устраивает TThread? И вообще, какова цель своей реализации потоковых классов?
Я к тому, что у меня есть небольшие наработки в этом направлении, быть может пригодится?

← →
panov © (2004-11-11 20:58) [10]

фух... сейчас буду думать и отвечать по порядку...

← →
panov © (2004-11-11 21:28) [11]

>Игорь Шевченко © (11.11.04 18:11) [1]

По-моему, стандарт и соглашение о передаче параметров в данном случае cdecl, а не stdcall.

>Игорь Шевченко © (11.11.04 18:12) [2]

Объясняю-)
Этот класс хочу использовать в пуле потоков. Он должен быть максимально простым и управляемым.

>Суслик © (11.11.04 18:13) [3]
1) зачем afterconstruction вызывать?

Вызывается для того, чтобы освободить поток(resume) в случае если он должен создаваться в неприостановленном состоянии.

2) не уверен, что верно, но я бы closehandle делал только если реально соответсвующий handle был инициализирован.

Логично. Сделаю проверку при создании потока.

3) зачем в aftercontruction ждать? ведь тогда повиснет вызывающий поток.

Для этого и сделано. Вызывающий поток должен быть уверен, что поточная функция начала работу до того, как начнутся обращения к объекту.

>DiamondShark © (11.11.04 18:26) [4]
BeginThread

В чем разница? -)

>DiamondShark © (11.11.04 18:30) [6]

Из конструктора вообще никогда не возвращаемся?

Возвращаемся после AfterConstruction в любом случае.

>DiamondShark © (11.11.04 18:36) [7]

А можно рассказать, зачем эти странные манипуляции с евентом?

Единственная цель - дождаться начала выполнения поточной функции.

>GuAV © (11.11.04 18:41) [8]

TThreadClass
IMHO следует придерживаться cтандартов на названия. У Вас надо TThreadObj или как-то так, а TXxxxClass - это class of TXxxx

FOnStartThread
Особенно здесь. Это болше похоже на поле события чем на метод.
Вносит путаницу при чтении кода.

Согласен, тут надо придумать получше навание.

ExitThread(Ret);
end;

Зачем так по-странному ? не проще ли вернуть результат ThreadProcedure, сделав её функцией.

А разве из поточной функции можно получить результат иначе как установив явно код в ExitThread?

procedure ThreadProcedure(const Sender: TObject); stdcall;
Можно сразу написать TThreadClass заместо TObject.

Возможно, но пока остановился на этом, так как(может быть) буду добавлять возможность замены поточной функции(дополнительно).

А можно даже объявить stdcall - метод класса (уже без параметра).

Пример передачи в CreateThread метода класса можно? - я с удовольствием так сделаю.

>Владислав © (11.11.04 18:57) [9]

Александр, можно поинтересоваться о предназначении этого класса? Чем не устраивает TThread? И вообще, какова цель своей реализации потоковых классов?
Я к тому, что у меня есть небольшие наработки в этом направлении, быть может пригодится?

В основной конференции была ветка про пул потоков. К сожалению, она сейчас ушла в архив, да и я не успел за это время проработать реализацию.

TThread не устраивает тем, например, что реализации его отличаются в 5-1 и 6-1 версии Delphi.

← →
GuAV © (2004-11-11 21:47) [12]

panov © (11.11.04 21:28) [11]
Пример передачи в CreateThread метода класса можно?
Таки сложно но можно. Сделал на примере метода формы но думаю понятно
type TForm1 = class(TForm) procedure FormCreate(Sender: TObject); private { Private declarations } public class procedure SomeMethod; end; var Form1: TForm1; implementation {$R *.dfm} const ThdMethod: procedure of object = TForm1.SomeMethod var ThdProc: pointer absolute ThdMethod; // это и передавать class procedure TForm1.SomeMethod; begin end;

AfterConstruction + F1:
AfterConstruction is called automatically after the object’s last constructor has executed. Do not call it explicitly in your applications.

panov © (11.11.04 21:28) [11]
А разве из поточной функции можно получить результат иначе как установив явно код в ExitThread?

ExitThread
The ExitThread function ends a thread.

VOID ExitThread(
DWORD dwExitCode // exit code for this thread
);
Parameters
dwExitCode
[in] Exit code for the calling thread. Use the GetExitCodeThread function to retrieve a thread"s exit code.

ThreadProc
....
Return Values
The function should return a value that indicates its success or failure.

Remarks
A process can obtain the return value of the ThreadProc of a thread it created with CreateThread by calling the GetExitCodeThread function. A process cannot obtain the return value from the ThreadProc of a thread it created with CreateRemoteThread.

← →
kaZaNoVa © (2004-11-11 21:49) [13]

"прямой" вызов BeginThread рулит ! (имхо, чтобы без заморочек с классами)

← →
GuAV © (2004-11-11 21:51) [14]

GuAV © (11.11.04 21:47) [12]
Пример с class procedure не самое лучшее, т.к. хотелось бы обычный.
Но идея та же: var TM: procedure of object; TM:=ThdMethod и потом typecase его в pointer.

← →
panov © (2004-11-11 21:56) [15]

>GuAV © (11.11.04 21:47) [12]
AfterConstruction is called automatically after the object’s last constructor has executed. Do not call it explicitly in your applications.

СОгласен, завтра протестирую этот момент.

Remarks
A process can obtain the return value of the ThreadProc of a thread it created with CreateThread by calling the GetExitCodeThread function. A process cannot obtain the return value from the ThreadProc of a thread it created with CreateRemoteThread.

Тоже проверю завтра, спасибо...

За пример с методом спасибо, но тебе не кажется, что это некоторое извращение? -)

← →
GuAV © (2004-11-11 22:00) [16]

Про cdecl вот
{$IFDEF MSWINDOWS}
function ThreadWrapper(Parameter: Pointer): Integer; stdcall;
{$ELSE}
function ThreadWrapper(Parameter: Pointer): Pointer; cdecl;
{$ENDIF}

За пример с методом спасибо, но тебе не кажется, что это некоторое извращение? -)

Согласен. Я просто упустил из вида момент что его надо куда то пердавать.

← →
Игорь Шевченко © (2004-11-11 22:07) [17]

panov © (11.11.04 21:28) [11]

> По-моему, стандарт и соглашение о передаче параметров в
> данном случае cdecl, а не stdcall.

А что для CreateThread F1 говорит ?
А что для BeginThread RTFS System.pas говорит ?

> Этот класс хочу использовать в пуле потоков. Он должен быть
> максимально простым и управляемым.

Я не понимаю без комментариев к твоему коду, каким образом он получается "простым и управляемым".

← →
panov © (2004-11-11 22:15) [18]

>GuAV © (11.11.04 22:00) [16]
>Игорь Шевченко © (11.11.04 22:07) [17]

про stdcall согласен. Не пойму, почему начал писать cdecl, ведь всегда stdcall писал.

Я не понимаю без комментариев к твоему коду, каким образом он получается "простым и управляемым".

Я завтра комментарии напишу, Игорь. Сегодня уже не успевал.

← →
GuAV © (2004-11-11 22:32) [19]

panov © (11.11.04 21:28) [11]
В чем разница? -)
А действительно в чём ?
пока нашел два отличия:
Установка Coprocesor Control Word
Установка переменной IsMultiThread

← →
Verg © (2004-11-11 22:38) [20]

Говрят велосипеды изобретать - способствует омоложению организЬма. :)

Это мне понравилось:

> procedure TThreadClass.Kill(ExitCode: Cardinal);
> begin
> TerminateThread(FHandle,ExitCode);
> Terminate;
> Free;
> end;

что-то зловещее, черное, стильное как семейка Адамсов :))

← →
jack128 © (2004-11-11 22:41) [21]

panov © (11.11.04 18:05)
TerminateThread(FHandle,ExitCode);
Terminate;
Free;

подчеркнутое - это припарка мертвому.

> procedure FOnEndThread;virtual;
этот метод - лишний. Тот же эффект достигается перекрытием Execute. И вообще - посмотрите еще

> 3) зачем в aftercontruction ждать? ведь тогда повиснет
>вызывающий поток.
>
> Для этого и сделано. Вызывающий поток должен быть
> уверен, что поточная функция начала работу до того,
> как начнутся обращения к объекту.

покажите, где у вас ивент в сигнальное состояни переходит??? Я лично не вижу, а значит основной поток висит пока ивент не будет уничтожен, то есть будет эждать пока доп. поток не отработает..

> Вызывается для того, чтобы освободить поток(resume) в
> случае если он должен создаваться в неприостановленном
> состоянии.
а не логичнее как в VCL сделать?? просто переоватьсоответствующий флаг в CreateThread

>BeginThread
>
> В чем разница? -)
дает знать менеджер памяти о том, что приложение - многопоточное. Ну и по мелочи, может от версии дельфи что то зависит..
panov © (11.11.04 21:28) [11]
TThread не устраивает тем, например, что реализации его отличаются в 5-1 и 6-1 версии Delphi.
скопируйте любую реализацию, а не выдумывайте свою глючную ;-) (шутка, но в любой щутке .. далее по тексту..)
GuAV © (11.11.04 22:32) [19]
Установка переменной IsMultiThread
это очень много!!!!

← →
Verg © (2004-11-11 22:42) [22]

Не хватает слов - "и по ветру разбросать прах твой"...

← →
jack128 © (2004-11-11 22:42) [23]

jack128 © (11.11.04 22:41) [21]
этот метод - лишний. Тот же эффект достигается перекрытием Execute. И вообще - посмотрите еще подчеркнутое не читать.

← →
jack128 © (2004-11-11 22:48) [24]

вообще единственное, что меня не устраивает в VCL реализации потока - это не виртуальность Terminate. Поскольку часто нужно давать потоку сигнал о том? что пора помирать не флагом FTerminated, а сигналом от ивента.

← →
Игорь Шевченко © (2004-11-11 23:00) [25]

> вообще единственное, что меня не устраивает в VCL реализации
> потока - это не виртуальность Terminate. Поскольку часто
> нужно давать потоку сигнал о том? что пора помирать не флагом
> FTerminated, а сигналом от ивента.

Напиши наследника и будет счастье

← →
Cobalt © (2004-11-11 23:06) [26]

jack128 © (11.11.04 22:48) [24]
Это, по-моему, излишество :)
Признак завершения работы - пожалуй, но как признак "убития" - "Право, это черезчур" :)

← →
panov © (2004-11-11 23:24) [27]

>Verg © (11.11.04 22:38) [20]

что-то зловещее, черное, стильное как семейка Адамсов :))

Ну вообще-то убить поток еще не значит освободить объект-оболочку-)

>GuAV © (11.11.04 22:32) [19]

IsMultiThread упустил из вида, действительно.

>jack128 © (11.11.04 22:41) [21]

TerminateThread(FHandle,ExitCode);
Terminate;
Free;

подчеркнутое - это припарка мертвому.

Здесь устанавливается FTerminated для того, чтобы для любого внешнего обращения (Terminated) до освобождения объекта было возвращено Terminated=True

Покажите, где у вас ивент в сигнальное состояни переходит??? Я лично не вижу, а значит основной поток висит пока ивент не будет уничтожен, то есть будет эждать пока доп. поток не отработает..

procedure TThreadClass.FOnStartThread; begin if not FSuspended then SetEvent(FEvent); end;

Про IsMultiThread уже выше написал-)

← →
Игорь Шевченко © (2004-11-11 23:30) [28]

panov © (11.11.04 23:24) [27]

> > procedure TThreadClass.Kill(ExitCode: Cardinal);
> > begin
> > TerminateThread(FHandle,ExitCode);
> > Terminate;
> > Free;
> > end;

В контексте какого потока выполняется этот код ?

А за Free внутри метода нужно, IMHO, руки отрывать сразу.

>Игорь Шевченко © (11.11.04 23:30) [28]

Free вызывается в контексте любого потока.

А за Free внутри метода нужно, IMHO, руки отрывать сразу.

А чем здесь плох вызов Free с учетом того, что далее отработает только Destroy?

> [28] Игорь Шевченко © (11.11.04 23:30)

> В контексте какого потока выполняется этот код ?

"Может выполняться".

Если в контексте самого FHandle, то чудные чудеса можно получить. В том числе и мемликов и всяких прочих "чудес"...

А вообще потоки "убивать" нельзя. Это должно быть железным правилом. Поток должен быть достаточно послушен, чтобы прекратить за приемлимое время свое существование самостоятельно по команде "менеджера".

GuAV © (11.11.04 23:36) [30]

Аргументирую: у меня есть переменная, указывающая на некий объект. Я вызываю некий метод, после которого переменная становится недействительной. Достаточно ?

Игорь Шевченко © (11.11.04 23:40) [32]
Достаточно ?

А метод Free это ведь тоже некий метод ?

Хотя конечно можно было бы сделать метод Kill desturctorом.

GuAV © (11.11.04 23:43) [33]

Вот метод Free является одним таким исключением, причем для любого объекта.
Любой другой метод, обладающий подобным побочным эффектом - кандидад в Recycle Bin.

Игорь Шевченко © (11.11.04 23:53) [34]
Вот метод Free является одним таким исключением

TThread.Resume и TCustomForm.Release могут также обладать таким эффектом.

GuAV © (12.11.04 00:07) [35]

> TCustomForm.Release

Доказать можешь ? Что после вызова Release указатель на форму в следующей команде станет недействительным.

> TThread.Resume

Да, при FreeOnTerminate. C этим согласен. Более того, даже TThread.Create. Кстати, при FreeOnTerminate освобождение объекта происходит в контексте порожденного потока, следовательно, указатель на него вряд ли стоит хранить.

Игорь Шевченко © (12.11.04 0:16) [36]
в следующей команде

В следующей - нет. Но после Application.Processmessages - запросто.

Игорь Шевченко © (12.11.04 0:16) [36]
> Доказать можешь ? Что после вызова Release указатель
> на форму в следующей команде станет недействительным.

ну очевидно не на следующей, но какая разница? Если это действие документировано. Да с Release локальная переменная никогда не станет инвалидной, но, например, поле объекта - станет.

Вообще - имеет ли объект право уничтожать себя сам?

Игорь Шевченко © (12.11.04 0:16) [36]
Более того, даже TThread.Create.

А вот это уже никак :-) TThread.Create не устанавливает FreeOnTerminate => он остаётся False. Что касается наследников, то они не изменяют TThread.Create а просто скрывают его, т.к. он не виртуальный

GuAV © (12.11.04 0:21) [37]
Application.Processmessages
хм, да. Но это все таки тоже исключение. С этом методом надо очень осторожно оброщаться.

Нужна критика кода-) Найти похожие ветки