Здравствуйте уважаемые мастера.
Что то у меня не совсем правильно работает мой сервер осованный на TServerSocket. Есть пара вопросов по поводу класса TServerThread. Стоит ли его перегружать внутренними функциями, или лучше сделать большую функцию ClientExecute.
Стоит ли добавлять в класс компоненты, допустим, для связи с бд, или создать их в функции?

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

constructor TServerThread.Create(CreateSuspended: Boolean; ASocket: TServerClientWinSocket;
 DataBaseName: string; DBUser: string; DBPassword: string);
begin
 inherited Create(CreateSuspended, ASocket);
 List := TStringList.Create;
 LogType := '';
 DataBase := TIBDatabase.Create(nil);
 Transaction := TIBTransaction.Create(DataBase);
 Proc := TIBStoredProc.Create(DataBase);
 Query := TIBQuery.Create(DataBase);
 try
   DataBase.DatabaseName := DataBaseName;
   DataBase.Params.Add('user_name=' + DBUser);
   DataBase.Params.Add('password=' + DBPassword);
   DataBase.LoginPrompt := false;
   DataBase.SQLDialect := 1;
   DataBase.DefaultTransaction := Transaction;
   DataBase.Connected := true;
   Transaction.Active := true;
   Query.Database := DataBase;
   Query.Transaction := Transaction;
   Proc.Database := DataBase;
   proc.Transaction := Transaction;
 except
   on e: Exception do
     LogError('[' + DateToStr(Now) + ']:::::: ThreadCreate::::' + e.Message);
 end;
 List.Add('[' + DateToStr(Now) + ']::: Thread created sucefuly');
end;

procedure TServerThread.ClientExecute;
var
 i, j: integer;
 buf1: array[0..2] of byte;
 c, cz, temp: byte;
 MessTest: TDisplayMesage;
begin
 inherited FreeOnTerminate := TRUE;
 try
   fSocketStream := TWinSocketStream.Create(ClientSocket, ssTimeOut);
   try
     while (not Terminated) and (ClientSocket.Connected) do
     begin
       if (not Terminated) and (not fSocketStream.WaitForData(ssTimeOut)) then
       begin
         List.Add('[' + DateToStr(Now) + ']::: Thread closed by timeout ');
         ClientSocket.Close;
         fSocketStream.Free;
         Free;
       end;
       DevNo := ClientSocket.ReceiveText;
       DevNo := Trim(DevNo);
       List.Add('[' + DateToStr(Now) + ']::: Thread recive number ');
               if DevNo = 'TEST' then
                 ClientSocket.SendText('OK')
               else
       begin
         if DeviceInDB then //function TServerThread.DeviceInDB:Boolean Check board is present =============
         begin
           if UpdateTime then //function TServerThread.UpdateTime:Boolean Check update time      =============
           begin
             CreatePackedge; //procedure TServerThread.CreatePackedge; Getting record for this board ======
             for j := 0 to 18 do //===================== Sending Erase command =============
             begin
               temp := clear_command[j];
               ClientSocket.SendBuf(temp, 1);
             end;
             List.Add('[' + DateToStr(Now) + ']::: Sending erase command ' + DevNo);
             for j := 0 to Length(mess) - 1 do //===================== Sending Messages ==================
               for i := 0 to Length(mess[j].PrepereMasage) - 1 do
                 ClientSocket.SendBuf(mess[j].PrepereMasage[i], 1);
             List.Add('[' + DateToStr(Now) + ']::: Sending packadge ' + DevNo);
             if (not Terminated) and (not fSocketStream.WaitForData(ssTimeOut * 5)) then //================ Waiting For Answer =====================
             begin
               List.Add('[' + DateToStr(Now) + ']::: Closed by time out on recive message count ' + DevNo);
               ClientSocket.Close;
               fSocketStream.Free;
               Free;
             end;
             cz := ClientSocket.ReceiveLength; //================ Reciving Message Count =================
             ClientSocket.ReceiveBuf(buf1, cz);
             List.Add('[' + DateToStr(Now) + ']::: resived message count ' + DevNo);
             if buf1[2] >= MessCount then //================ SetUP update time ======================
             begin
               SetUpUpdateTime;
               logstr := 'Äèñïëåé ïîäêëþ÷èëñÿ è ïîëó÷èë ' + IntToStr(buf1[2]) + ' ôàéëîâ.' + #13#10;
               LogType := 'UpdateOK';
               LogConnection;
             end
             else
             begin
               logstr := 'Äèñïëåé ïîäêëþ÷èëñÿ íî ïîëó÷èë ' + IntToStr(buf1[2]) + ' ôàéëîâ, à íå ' + IntToStr(MessCount + 2);
               LogType := 'TrasferFaild';
               LogConnection;
             end;
           end; //================ Closing Conection =================
           if LogType = '' then
           begin
             logstr := 'Äèñïëåé ïîäêëþ÷èëñÿ, íî âðåìÿ îáíîâëåíèÿ åù¸ íå ïðèøëî.';
             LogType := 'NoTime';
             LogConnection;
           end;
         end
         else
         begin
           MessTest := TDisplayMesage.Create;
           MessTest.CR;
           MessTest.PrepMess;
           for i := 0 to Length(MessTest.PrepereMasage) - 1 do
             ClientSocket.SendBuf(MessTest.PrepereMasage[i], 1);
           List.Add('[' + DateToStr(Now) + ']::: Sending number for device out DB ' + DevNo);
         end;
         if LogType = '' then
         begin
           logstr := 'Ïîäêëþ÷åííîå óñòðîéñòâî îòñóòñòâóåò â áàçå äàííûõ.';
           LogType := 'NoDevice';
           LogConnection;
           ClientSocket.Close;
         end;
         List.Add('[' + DateToStr(Now) + ']::: Connection close It''s OK ' + DevNo);
         LogServerEvents(List);
         ClientSocket.Close;
       end;
     end;
   finally
     List.Add('[' + DateToStr(Now) + ']::: Finnaly ' + DevNo);
     fSocketStream.Free;
     Free;
   end;
 except
   on e: exception do
   begin
     LogError('[' + DateToStr(Now) + ']::: ThreadExecute::::' + e.Message);
     ClientSocket.Close;
     Terminate;
   end;
 end;

end;

> лучше сделать большую функцию ClientExecute


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


> или создать их в функции


Именно.
Особенно в твоем случае с потоконебезопасными объектами доступа к IB.


> DataBase.Connected := true;


Засада поджидает уже в этой строчке - коннект ты осуществляешь в дополнительном слушающем потоке сервера, вызвавшем конструктор, а последующую работу с базой в теле  ClientExecute ты осуществляешь уже в другом доп.потоке, ассоциированном со вновь установленным сокетным соединением.


> FreeOnTerminate := TRUE;


Для объектов-наследников TServerClientThread "саморазрушение" недопустимо - сервер сам управляет временами жизни объектов-потоков, регистрируемых им во внутреннем кэше после конструирования.

И вообще - твоя логика работы с методами приема/передачи объекта TCustomWinSocket не выдерживает никакой критики.
И доп.потоки тут ни причем - при такой логике граблей можно нахватать в любом потоке.

dozcent ©   (07.08.08 19:53)
Стоит ли его перегружать внутренними функциями
Стоит! без паллитры в этом листинге не разберешься
dozcent ©   (07.08.08 19:53)
Стоит ли добавлять в класс компоненты
У где они у тебя? наруже? нипорядок!!!
1. нитрогай constructor TServerThread.Create... перенеси все (создание конектов и прочей фегни) в ClientExecute, оставить можешь только присвоение DataBaseName/DBUser/DBPassword во внутренние переменные и пользовать их в ClientExecute...
2. inherited FreeOnTerminate := TRUE; - НИЗЯ!!!
3. Free; - АХТУНГ! НИЗЯ!
4.

А я тебе фсе телевидение испорчу :) послав более SizeOf(buf1) и получишь AV или уязвимость с выполнением вредоносного кода
5.


итого налицо двойное убийство, с особой тяжестью... приговор отрубление кривых рук сроком на 10 лет :)
6. Протокол основан на честном слове - никаких проверок на размеры и соответствие полей, а в сочетании с сокетами которые умеют клеить и дробить пакеты ты огребеш... и в DevNo := ClientSocket.ReceiveText; ты получиш или пол сообщения или несколько

dozcent ©   (07.08.08 19:53)
PrepereMasage
Мнебы сейчас массажистку...
кОкай языг учiлЪ?

Короче говоря, в приведенном коде косяк на косяке косяком погоняет)

Проще выкинуть его в топку и переписать "с нуля" - с чувством, с толком, с расстановкой)

Разъясните тогда пожалуйста, кто когда и почему удаляется в этой конструкции, а то я что-то непойму...
Если у меня


Я вызываю её создание в ClientExecute, то когда её нужно убить...
И почему нельзя
FreeOnTerminate := TRUE;
Что происходит при ClientSocket.Close кроме закрытия сокета?

Вот тут кое что подправил, появилась куча ошибок и вобще что попало..


Что то мне непонятно, когда же эти потоки убиваются!

> if cz <> 17 then//если длинна не равна 17, то это не моё
> устройство!!!


TCP - протокол поточный !

тебе это о чем-то говорит ?

> TCP - протокол поточный !

В данном контексте ни о чём не говорит....

Меня больше волнует, почему при имеющемся exception у меня вылазит ошибка!!!!

О контексте ты не сказал ни слова.


> if cz <> 17 then//если длинна не равна 17, то это не моё
> устройство!!!


С какого перепугу-то ?

Мало ли сколько байт принятов ДАННЫЙ момент времени ?

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


> почему при имеющемся exception у меня вылазит ошибка!


Куда она у тебя "вылазит" ?

Куда, куда - на экран...
Чесно говоря, сейчас не до гипотетических ошибок, эти 17 байт посылает модем неизвесно каким образом, и они всегда доходят в 100% случаев...
Но очень интересно посмотреть как принять их правильно.
сейчас мне нужно что бы ошибки не останавливали работу сервера и не вылазили на экран, я их ловлю во всех функциях, а они вылазят на экран....
И немогли бы вы кратко объяснить принцип удаления потока из памяти в данной схеме?

Да, и ещё....
Непонятное мне явление...
В блоке finally есть обращение к процедуре LogServerEvents('event');
которое в некоторых потоках срабатывает два раза.....

> как принять их правильно


Их надо аккумулировать.


> 17 байт посылает модем


Какой еще нахрен модем ? Речь в твоем коде идет о сокете как об источнике поточных данных ..


> не до гипотетических ошибок


Какая еще нахрен "гипотетика", если источник в одном углу Тырнета, а приемник совсем в другом ?


> они всегда доходят в 100% случаев


Не надо уже трындеть.

Разнеси свои приемник и передатчик по разным углам Тырнета и твоя логика тут же потерпит крах.

И вообще - отладчик тебе в руки, коль ты не желаешь слушать что тебе говорят.

При чём тут нежелаю, я прошу помощи, что бы сказали что-то конкретное...
Что такое аккомулировать? Чем?
В цикле принимать данные, пока не наберёт 17?
Вы имеете в виду, что получать и посылать данные надо fSocketStream'ом?
Или что?
А какие мысли по поводу ошибки, которая на экран вылазит?

А когда я говорил про "ГИПОТЕТИКУ", я имел в виду, что такой проблеммы сейчас не существует, а вот ошибки!!!!!!!!! Которые на экран вылазят!!!!
И о которых я прошу помощи!!!

> В цикле принимать данные, пока не наберёт 17?


В цикле или не в цикле - не важно.

Но если ты ожидаешь от партнера по соединению 17 каких-то байт, то их сначала нужно гарантированно принять.


> cz := ClientSocket.ReceiveLength;


Читай внимательно справку по логике работы этого св-ва - твое понимание этой логики НЕверно.

> ошибки!!!!!!!!! Которые на экран вылазят


Да по барабану что там у тебя куда-то "вылазит" !

Ошибки в твоей логике начинаются гораздо раньше по ходу твоего алгоритма !

И рано или поздно они дадут о себе знать !

Пойми ты наконец-то это)

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

Нет аккумулятора, нет машины состояния - дальнейший разговор лишен смысла.

Как реализовать этот аккомулятор и машину состояния?

Вы имеете в виду такую конструкцию...

Даже для трёх байт такое надо делать?

> Даже для трёх байт такое надо делать?


Да, даже для двух.

Если тебе пообещали налить бутыль и ты ждешь обещанное, то это вовсе не означает,что ее нальют тебе все разом и сей секунд после обещания - тебе ее когда-нибудь обязательно нальют, но вправе наливать ее через заранее неизвестного/меняющегося диаметра воронку и заранее неизвестными порциями.

Так что готовь тару объемом в бутыль и жди пока она наполнится, коль договорились)

Время наполнения и порции, коими наполнение должно происходить, в общем случае тебе не известно)

> FillChar( fRequest, REQUESTSIZE, 0 );


А это вообще нафиг не нужно.

А в этой функции лучше ставить минимум или максимум?
  readlen := fSocketStream.Read( fRequest[ac],1024 );
Тобишь 1 или 17?

приведи протокол устройтсва...

dozcent ©   (10.08.08 23:26) [21]
Вы имеете в виду такую конструкцию...
почти

Должно получиться чтото типа этого:

> Тобишь 1 или 17?


Читай ровно столько, сколько осталось прочитать из этих 17-ти ожидаемых байт

запросил к чтению 17, фактически прочитал 2, осталось прочитать 15
запросил  к чтению 15, фактически прочитал 5, осталось прочитаь 10
запросил  к чтению 10, фактически прочитал 1, осталось прочитаь 9
запросил  к чтению 9, фактически прочитал 4, осталось прочитаь 5

и т.д. до тех пор пока "осталось прочитаь" не станет равным 0

Эта логика применима во всех случаях, когда заранее известен размер очередного инф.сообщения, ожидаемого к приему от партнера на данном этапе исполнения протокола инф.обмена.