Рефакторинг исходников шлюза

← →
Сатир (2007-06-04 12:56) [0]

Всем привет.
Достались по наследству полурабочие исходники сервиса шлюза.
Его предназначение - принимать данные из SOLVO и отправлять в Oracle.
В этой ветке http://delphimaster.net/view/15-1179906322/ начало этой проблеммы.
А сейчас, после того, как переход на десятку не решил проблемму, хотелось бы обсудить со знатоками проблеммы проектирования сего творения.
Вообщем, в двух чертах как оно выглядит.
Есть сервис, наследованный от TService.
Есть три датамодуля:
1) UDM - модуль с оракловой сессией и реализацией ХП для заливки в оракл
2)uSendDM - модуль для отправки данных, на нем расположен idTCPServer, который висит на 29002 порту
3)uRecDM - модуль для приёмки данных, на котором тоже расположен idTCPServer, который висит на 29001 порту

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

После двух недель медитирования над этим детищем возникло подозрение, что оба многопоточных сервера, которые живут в одном адресном пространстве шлюза, каким-то образом в определённый момент(после перезапуска системы SOLVO и переподключения к шлюзу) теряют свои потоки и зацикливаются на этом куске кода:

файл IdIdServerIOHandlerSocket.pas пакета Indy 10

function TIdServerIOHandlerSocket.Accept(
ASocket: TIdSocketHandle;
AListenerThread: TIdThread;
AYarn: TIdYarn
): TIdIOHandler;
var
LIOHandler: TIdIOHandlerSocket;
begin
LIOHandler := IOHandlerSocketClass.Create(nil);
LIOHandler.Open;
Result := nil;
while not AListenerThread.Stopped do try
if ASocket.Select(250) then begin
if LIOHandler.Binding.Accept(ASocket.Handle) then begin
LIOHandler.AfterAccept;
Result := LIOHandler;
Break;
end else begin
FreeAndNil(LIOHandler);
Break;
end;
end;
finally
if AListenerThread.Stopped then begin
FreeAndNil(LIOHandler);
end;
end;
end;

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

Посему возникло два решения проблеммы:
1) развести приёмку и отправку на два отдельных сервиса, каждый из которых будет висеть на своём порту, оба будут общаться через те же общие папки или возможно лучше сделать как-то по-другому их взаимодействие
2) Убрать idTCPServer из каждого модуля и завести один общий на двоих
и динамически менять ему номер порта
3) третий вариант

Заранее благодарен за ваши отзывы.

← →
Eraser © (2007-06-04 13:01) [1]

> [0] Сатир (04.06.07 12:56)

В OnExecute на сервере есть Exit до выполнения первой инструкции передачи данных?

← →
Eraser © (2007-06-04 13:01) [2]

> 2) Убрать idTCPServer из каждого модуля и завести один общий
> на двоих
> и динамически менять ему номер порта

idTCPServer может одновременно слушать несколько портов :))

← →
Сатир (2007-06-04 13:24) [3]

> В OnExecute на сервере есть Exit до выполнения первой инструкции
> передачи данных?

вот обработчик OnExecute модуля приёмки данных:
Procedure TRecDM.IdTCPServerExecute(AContext: TIdContext {AThread: TIdPeerThread}); Var ActClient: PClient; InString: String; lBackMsg: TBackMsg; Begin Try If Not TIdThread(AContext.Yarn).Terminated And AContext.Connection.Connected Then Begin ActClient := PClient({AThread}AContext.Data); ActClient.LastAction := Now; End; (*прочитать строку*) InString := {AThread.Connection.ReadLn()} AContext.Connection.IOHandler.ReadLn; (*не шапка ли?*) If AnsiSameText(Trim(InString), "<?xml version="1.0" encoding="windows-1251"?>") Then InputMsg.Clear; (*не конец ли?*) If POS("</packetinfo>", InString) <> 0 Then Begin (*если таки конец пакета*) InputMsg.Add(InString); lBackMsg := CreateXMLFile(InputMsg); DM.WriteTMPlog(lBackMsg, 2, "RecDM"); End Else (*иначе просто записываем/дописываем новую строку*) InputMsg.Add(InString); Except On E: Exception Do Begin DM.WriteTMPlog("E: Ошибка чтения с канала: " + E.Message, 3, "RecDM"); End; End; End;

а вот обработчик OnExecute модуля отправки данных:
Procedure TSentDM.IdTCPServerExecute2(AContext: TIdContext {AThread: TIdPeerThread}); Var ActClient: PClient; Begin Try If Not TIdThread(AContext.Yarn).Terminated And AContext.Connection.Connected Then Begin ActClient := PClient(AContext.Data); ActClient.LastAction := Now; End; InString := ""; InString := AContext.Connection.IOHandler.ReadLn(); DM.WriteTMPlog("Msg--> " + InString, 4, "SentDM"); If POS("<error", InString) <> 0 Then Begin If POS("/>", InString) = 0 Then InString := InString + """ + "/>"; TIdThread(AContext.Yarn).Synchronize(WorkWithErrorMsg); End; If POS("<done", InString) <> 0 Then TIdThread(AContext.Yarn).Synchronize(WorkWithDoneInMsg); Except On E: Exception Do DM.WriteTMPlog("E: [IdTCPServerExecute]: " + E.Message + "{" + InString + "}", 3, "SentDM"); End; End;

Где тут Exit прилепить?

← →
Сатир (2007-06-04 13:26) [4]

> idTCPServer может одновременно слушать несколько портов
> :))

а можно об этом поподробней, плиз

← →
Eraser © (2007-06-04 13:54) [5]

> [4] Сатир (04.06.07 13:26)

да мало ли где, всякое бывает )

> а можно об этом поподробней, плиз

см. св-во bindings.

← →
Сатир (2007-06-04 14:00) [6]

> да мало ли где, всякое бывает )

да уж, содержательный ответ:))

> см. св-во bindings.

ну это свойство мне знакомо, только не совсем понятно, как с ним работать
есть реальные примеры?
или тоже "да мало ли как, можно повсякому"?:-)

← →
Eraser © (2007-06-04 14:03) [7]

> [6] Сатир (04.06.07 14:00)

хм.. ну как работать, прямо в IDE можно. А можно и в runtime. Неужели это проблема? на сколько помню там все предельно прозрачно, дабавляешь пары IP-port и все работает.

← →
SlymRO © (2007-06-04 14:50) [8]

Сатир (04.06.07 13:24) [3]
Это, я в ступоре... а где while not Terminated do?
Код будет работать если только посылочка придет одним пакетиком, цыкла продолжения чтения то нет ;)

← →
SlymRO © (2007-06-04 14:56) [9]

Procedure TRecDM.IdTCPServerExecute(AContext: TIdContext);
Var
ActClient: PClient;
InString: String;
lBackMsg: TBackMsg;
InputMsg:TStringList;
Begin
try
InputMsg:=TStringList.Create;
Try
while AContext.Connection.Connected do
begin
ActClient := PClient(AContext.Data);
ActClient.LastAction := Now;
End;
InString:=AContext.Connection.IOHandler.ReadLn;
If AnsiSameText(Trim(InString), "<?xml version="1.0" encoding="windows-1251"?>") Then
InputMsg.Clear;
InputMsg.Add(InString);
If POS("</packetinfo>", InString) <> 0 Then
Begin
lBackMsg := CreateXMLFile(InputMsg);
DM.WriteTMPlog(lBackMsg, 2, "RecDM");

AContext.Connection.Disconnect;//Можно дисконекнуться
End;
finally
InputMsg.Free;
end;
Except
On E: Exception Do
Begin
DM.WriteTMPlog("E: Ошибка чтения с канала: " + E.Message, 3, "RecDM");
End;
End;
End;

← →
SlymRO © (2007-06-04 15:03) [10]

If AnsiSameText(Trim(InString), "<?xml version="1.0" encoding="windows-1251"?>")
If POS("</packetinfo>", InString) <> 0
некошерно так делать:
Что будешь делать с этим: <?xml encoding="windows-1251" version="1.0" ?>
или с этим </Packetinfo>
+я злоумышленник задосю твой сервер послав 1 млн строк или 2ГБ инфы и твой сервер сдохнет, от недостатка памяти
Протокол взаимодействия не продуман!!!
1. Необходимо передавать длинну посылки!
2. Ограничить размер посылки
3. Определять начало конец посылки не по сигнатурам а по началу коннекта и длинне посылки из заголовка

← →
Сатир (2007-06-04 15:40) [11]

> дабавляешь пары IP-port и все работает.

так бы и сказал:"опыта разработки подобных приложений не имею. так, читал де-то...":-)
но всё равно спасибо, что отметился:)))

> Протокол взаимодействия не продуман!!!

согласен

> 1. Необходимо передавать длинну посылки!
> 2. Ограничить размер посылки
> 3. Определять начало конец посылки не по сигнатурам а по
> началу коннекта и длинне посылки из заголовка

спасибо за замечания, но сейчас стоит задача в другом
уточню: как заставить шлюз продолжать нормально работать после перезагрузки SOLVO, которое до этого установило с ним коннект?
Поэтому предлагаю сосредоточиться именно на этом.
Если интересует какая-то дополнительная инфа, готов её безусловно предоставить.

← →
SlymRO © (2007-06-04 15:52) [12]

Судя по всему (если это работает):
1. RecDM: на один xml одно соединение, принял файл разединился
2. тоже самое с SentDM, принял разединился.
я это решил не узрев ниодного цикла...
или я что то не понял? если так то
1. на каком основании ты решаешь в SentDM что за один ReadLn ты принял всю xml?
2. и как ты дочитываешь останки xml в RecDM

← →
Сатир (2007-06-04 16:55) [13]

> Судя по всему (если это работает):
> 1. RecDM: на один xml одно соединение, принял файл разединился
> 2. тоже самое с SentDM, принял разединился.

нет, не так.
Он соединяется и находится в таком состоянии, пока работает на той стороне SOLVO - это такая программа под линуком, которая время от времени просматривает заказы и формирует по изменениям xml, который шлёт на шлюз. Поэтому, связь со шлюзом есть, когда запущена эта прога.
А дисконнект с ним происходит, когда эта программа перестаёт работать на стороне линукса.

> 1. на каком основании ты решаешь в SentDM что за один ReadLn
> ты принял всю xml?

не всю, а только одну строчку
он в одном пакете шлёт только одну строку файла, и на стороне шлюза собирает воедино. Поэтому, каждый вызов OnExecute - это одна строка xml-файла и по анализу тегов можно определить, когда файл начался, а когда закончился.
Это не я проектировал, это так досталось мне по наследству.
Возможно, лучше было бы как-то по другому.
Если есть какие-то конструктивные пожелания, буду рад их выслушать.

← →
SlymRO © (2007-06-04 17:08) [14]

Сатир (04.06.07 16:55) [13]
каждый вызов OnExecute
OnExecute не ка каждый вызов, а на 1 соединение! как только покинул OnExecute сервер закрывает соединение!!!
в OnExecute должен быть вечный цикл! постоянно читающий

← →
SlymRO © (2007-06-04 17:10) [15]

Мне делать зафтра вроде нефик, можешь кинуть в меня исходником посмотрю поправлю

← →
Eraser © (2007-06-04 17:12) [16]

> [11] Сатир (04.06.07 15:40)

> так бы и сказал:"опыта разработки подобных приложений не
> имею. так, читал де-то...":-)

мне виднее опыт разработки каких приложений я имею.

← →
Сатир (2007-06-04 17:20) [17]

> SlymRO © (04.06.07 17:10) [15]

а мыло рабочее?

← →
Сатир (2007-06-04 17:21) [18]

> мне виднее опыт разработки каких приложений я имею

ты что, обиделся?:)))
ну извини, если задел...честно, не хотел тебя обидеть:-)

← →
Kolan © (2007-06-04 17:22) [19]

Если ты хочешь сделать именно рефакторинг, то паказывай код.

← →
Сатир (2007-06-04 17:27) [20]

> Если ты хочешь сделать именно рефакторинг, то паказывай
> код.

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

← →
Kolan © (2007-06-04 17:32) [21]

Просто то о чем тут идет речь(в сабже я кстати не разбираюсь)  это перепроектирование.

А рефакторинг это нечто другое.
Если смотреть код [0], то тут хотябы Extract Method.
И избавится от запаха Long Parameter List.

← →
Kolan © (2007-06-04 17:37) [22]

> Long Parameter List.

С этим я поторопился наверно.

← →
Kolan © (2007-06-04 17:39) [23]

> Long Parameter List.

С этим я поторопился наверно.

← →
Сатир (2007-06-04 17:46) [24]

> Если смотреть код [0],

этот код рефакторить не нужно.
За этот код отвечают ребята с сайта http://www.indyproject.org
У них там ведётся свой bug-list и все замечания лучше переправлять туда.
В этой ветке обсуждается только архитектура шлюза, написаного с использованием компонентов Indy 10.

> В этой ветке обсуждается только архитектура шлюза, написаного
> с использованием компонентов Indy 10.

А тему назвали рефакторинг :)
По сабжу
If POS("</packetinfo>", InString) <> 0 Then
Вот эти все моменты надо прорефакторить с помощью Extract Method. Чтобы получилось
if IsEndOfPackage(InString) then

← →
Сатир (2007-06-04 18:16) [27]

> Глобальная переменная что-ли?

приватная переменная класса

> приватная переменная класса

Где буква F вначеле?
-Обязательно поставить

Так она используется только для обработки сообщения? Лучьше имхо делать её локальной.

жесть какая... Чтобы такого не случалось и придумали системы интеграции типа BiztakServer или WebSphere.... Посоветуй начальству использовать уже готовые решения для разработки систем интеграции, заодно нехило поднимешь свой уровень.
Меня как-то пытались затащить на работу по доработке и сопровождению самописного сервера интеграции - я посмотрел на их схему бизнес-процесса и сказал - нафиг-нафиг :)

← →
Сатир (2007-06-04 18:45) [30]

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

ага, посоветуй... когда на эту разработу угробили кучу человеко-ресурсов и времени, поди посоветуй... порвут на месте.

> нафиг-нафиг :)

да, ладно, как-нить разгребу, помедитирую ещё недельку-вторую, что-то перепишу, что-то допишу, и хотя бы какой-то приемлемый уровень будет достигнут в любом случае:-)

> ага, посоветуй... когда на эту разработу угробили кучу человеко-
> ресурсов и времени, поди посоветуй... порвут на месте.

и еще кучу убьете. Хорошо если работать будет стабильно какое-то время. А после того как ты разберешься с этим, через некоторое время свалишь и все начнется сначала :) Тем более что интеграционные системы имеют привычку быстро расти и без предварительного ТЗ :) И рефакторить код зачастую нет времени - со времен получается монстр. А если еще учесть что стоимость ошибки в таких системах часто очень высока, то подумайте лучше хорошенько.

← →
Сатир (2007-06-04 19:02) [32]

оно уже сейчас работает, только нужно синхронно перезапускать вручную вместе с перезапуском той системы, которая устанавливает с ним коннект
а хотят от этого избавиццо
вот и вся задача
так что скорее всего будет два сервиса(приёмки и отправки), а там посмотрим

ты случайно не в Москве и не на Солянке работаешь в ВиммБильДаме?

← →
Сатир (2007-06-04 19:27) [34]

> ты случайно не в Москве и не на Солянке работаешь в ВиммБильДаме?

неа, киевский я...

Рефакторинг исходников шлюза Найти похожие ветки