View Issue Details

Jump to NotesJump to History
IDProjectCategoryView StatusDate SubmittedLast Update
0003957JEDI VCL00 JVCL Componentspublic2006-10-15 15:072007-01-04 04:20
ReporterAriochAssigned Toobones 
PrioritynormalSeveritytweakReproducibilityalways
Status resolvedResolutionwon't fix 
Product VersionDaily / GIT 
Target VersionFixed in Version 
Summary0003957: JvCommStatus - code tuning
Description1) why should watcher thread try to run, when handle is 0 ? if handle would be changed to non-zero - it would be resumed explicitly, like following:

procedure TJvCommWatcher.Execute;
var
  Mask: Cardinal;
begin
  // (rom) secure thread against exceptions
  try
    while not Terminated do
    begin
      if FHandle <> 0 then
      begin
        GetCommModemStatus(FHandle, Mask);
        if Mask <> FStat then
        begin
          FStat := Mask;
          Synchronize(Changed);
        end;
      Sleep(50); // <- **************
      end // <- **************
      else Suspend; // <- **************
    end;
  except
  end;
end;


Also this potentially lets the thread to be Freed on some exception within Synchronize(Changed) - and dead pointer will be kept within JvCommStatus



2) strange places in
procedure TJvCommStatus.SetComm(const Value: TJvCommPort);
var
  Stat: Cardinal;
  CommName: string;
begin
  if FWatcher <> nil then
    FWatcher.FHandle := 0;
  if FHandle <> 0 then
    CloseHandle(FHandle);
  FHandle := 0; //******** 1 ***
  FComm := Value;
  // (rom) simplified through better TJvCommPort
  if FComm <> 0 then
  begin
    CommName := 'COM' + IntToStr(FComm);
    FHandle := CreateFile(PChar(CommName), 0, FILE_SHARE_READ or FILE_SHARE_WRITE, nil, OPEN_EXISTING, 0, 0);
  end;

  if GetCommModemStatus(FHandle, Stat) then //****2***
    UpdateStates(Stat)
  else
    UpdateStates(0);

  if FWatcher <> nil then
  begin
    FWatcher.FHandle := FHandle;
    FWatcher.FStat := 0; // ******3***
    if FHandle <> 0 then
      FWatcher.Resume
    else
      FWatcher.Suspend;
  end;
  OnChange(Self); // ******4***
end;

***1*) would this ever be changed from var to property - this double assignment can potentially led to something unpredictable ( like issue in JvImagesViewer, when ObjectList[i] := ObjectList[i] caused the object to free but now-dead pointer be kept. Also it is an extra assignment sometimes.

***2*) hence, whether Handle is zero - we inteentionally call Win32 API with incorrect data ? And why, only to call OnChanged ?
And what is when Handle is INVALID_HANDLE_VALUE ? watcher thread will still try to read it.

***3*) Thus making almost sure, that next time watcher thread will work - it will almost sure fire an event ?

***4*) and here we call OnChanged once again. 3 times per port change - why ?
Values is another question - 1st event is real state, then 2nd - always zero, then some time after - watcher thread will bring new correct values.


One more moment - most properties of VCL do something only when they a really changed, so assigning them their previous value would do nothing.
Not so for JvCommStatus, seems assigninmg Comm property is always re-initing the component and always firing two or three events!

I think we should really think if we wanna fire two or three events when parent form is loading and not initialized completely yet. (and i wonder how good would Synchronize from worker thread work when the form is loading yet)



3) constructor TJvCommStatus.Create(AOwner: TComponent);
begin
  inherited Create(AOwner);
  FComm := 0;
  FHandle := 0;

  if not (csDesigning in ComponentState) then
  begin
    FWatcher := TJvCommWatcher.Create(True);
    FWatcher.FreeOnTerminate := True;

    FWatcher.FHandle := FHandle; //// always zero
    FWatcher.FStat := 0;
    FWatcher.FOnChange := OnChange;

    FWatcher.Resume; /// why ??? Handle is zero!!!
  end
  else
    FWatcher := nil;

  SetComm(FComm); // yet again set thread properties and `try` to fire event
end;


4) procedure TJvCommStatus.OnChange(Sender: TObject);
Won't it better to be called like WatcherChanged, to be easy dsitinguishable from OnChanged event ?
TagsNo tags attached.

Activities

2006-10-15 15:49

 

JvCommStatus.pas (7,449 bytes)

obones

2006-10-16 01:57

administrator   ~0010364

Please provide an example showing the issues, and a patch file as it makes integration easier.

obones

2007-01-04 04:20

administrator   ~0010527

No news, leaving the issue as is.

Issue History

Date Modified Username Field Change
2006-10-15 15:07 Arioch New Issue
2006-10-15 15:07 Arioch File Added: JvCommStatus.pas
2006-10-15 15:48 Arioch File Deleted: JvCommStatus.pas
2006-10-15 15:49 Arioch File Added: JvCommStatus.pas
2006-10-16 01:57 obones Note Added: 0010364
2006-10-16 01:57 obones Status new => feedback
2007-01-04 04:20 obones Status feedback => resolved
2007-01-04 04:20 obones Resolution open => won't fix
2007-01-04 04:20 obones Assigned To => obones
2007-01-04 04:20 obones Note Added: 0010527