Link to home
Start Free TrialLog in
Avatar of Wim ten Brink
Wim ten BrinkFlag for Netherlands

asked on

[Bugfix] Or a challenge... Find the bugs... :-)

I asked before if people could find any possible bugs in one of the units I wrote. In the meantime I've updated another unit that I'm working on and I want to be sure it's flawless too, but I just run out of time to test it thoroughly myself...

The unit is at http://www.workshop-alex.org/Sources/TrayIcon/untExtraInterfaces.html and basically, it's a wrapper around standard Windows objects, like a mutex, a semaphore, a critical section, an event and since today, the FindXXXChangeNotification API. There's also some kind of timer and an unique number generator in it but they aren't too interesting right now.

So, well... I share this code here for the EE members and will reward any bug found. 100 points for a major bug, 50 for a minor and 20 for an interesting suggestion. Points will be divided between participants once 5 major bugs, 10 minor bugs or 25 suggestions (or combinations) have been found. First comes, first goes...

If no bugs are found then consider the reward for your tests the use of some very interesting interfaces in your code. ;-) Maybe you can find some good use for them. The code is free of charge, as reward for testing. :-)

I hope the code explains itself a bit. It's advanced Delphi code so not real beginners stuff. It should encapsulate the more complex Windows API functions.

About the critical section interface, if you say 'Blocking=False' then it's actually doing nothing! Just a trick to check if code runs better without a critical section. (Or when the use of a CS depends on some setting.)

The IFolderChange interface is the one I'm most interested in. I'll show a simple example here of it's usage.

unit frmMain;

interface

uses
  ..., untExtraInterfaces;

type
  TFormMain = class( TForm )
    Memo: TMemo;
    procedure FormCreate( Sender: TObject );
    procedure FormDestroy( Sender: TObject );
  private
    FolderChange: IFolderChange;
  protected
    procedure Add( const Value: string ); overload;
    procedure Add( const Value: string; const Data: array of const ); overload;
    procedure HasChanged( const Path: string );
  public
  end;

var
  FormMain: TFormMain;

implementation

{$R *.DFM}

procedure TFormMain.Add( const Value: string );
begin
  Memo.Lines.Add( Value );
end;

procedure TFormMain.Add( const Value: string; const Data: array of const );
begin
  Add( Format( Value, Data ) );
end;

procedure TFormMain.FormCreate( Sender: TObject );
begin
  FolderChange := NewFolderChange( IncludeTrailingBackslash( ExtractFilePath( ParamStr( 0 ) ) + 'XML), HasChanged, FILE_NOTIFY_CHANGE_FILE_NAME or FILE_NOTIFY_CHANGE_SIZE );
end;

procedure TFormMain.FormDestroy( Sender: TObject );
begin
  FolderChange := nil;
end;

procedure TFormMain.HasChanged( const Path: string );
var
  SearchRec: TSearchRec;
  Source: string;
  Target: string;
  AFile: TStream;
begin
  Add( 'Files changed in "%s".', [ Path ] );
  if ( FindFirst( Path + '*.*', faAnyFile - faDirectory, SearchRec ) = 0 ) then begin
    repeat
      try
        Source := Path + SearchRec.Name;
        Sleep( 0 );
        TFileStream.Create( Source, fmOpenReadWrite + fmShareExclusive ).Free; // Just to check if we can open the file exclusively.
        DeleteFile( Source );
      except on E: Exception do Add( E.Message );
      end;
    until ( FindNext( SearchRec ) <> 0 );
    FindClose( SearchRec );
  end;
end;

end.

Explanation: Above example creates a file notifier for a subfoler 'XML' in my application folder. Whenever a file is dumped in this folder, it should try to see if it can be opened exclusively and then just delete the file. (Actually, I will be processing the contents before deleting it.) Since the notifier only tells me that a change has occurred and doesn't tell me what has changed exactly, I will have to loop through all files. Not a big problem since I will process everything in this folder...
If it fails to process a file (because it's still open) then no worries. It will get processed when I get notified about the next change in this folder.

Do keep in mind though, the HasChanged method is NOT executed in the main thread but in a separate thread. As long as HasChanged is running, the system isn't notified about other file changes. I could synchronise this code with the main thread but this should be done from the HasChanged method. Problem is, this code is also supposed to work in console applications where the main thread doesn't have a messageloop. Without a messageloop, it's not possible to start some function in the main thread...

And if this unit or code has been useful for you, just post a note here too. Or at the member feedback in my profile, if you want to inflate my ego... :-)

Remember, I am interested in bugs in this code and just don't have enough time to test it thoroughly myself. (Besides, no developer should test his own code!)
ASKER CERTIFIED SOLUTION
Avatar of BlackTigerX
BlackTigerX

Link to home
membership
This solution is only available to members.
To access this solution, you must be a member of Experts Exchange.
Start Free Trial
Avatar of BlackTigerX
BlackTigerX

forgot to mention, even better would be a parameter that indicates if you wish to create the folder
Avatar of Wim ten Brink

ASKER

Nice tip! And yes, a minor bug. I should check if the folder exists... Unfortunately, the function DirectoryExists is part of FileCtrl in Delphi 5 and not in the SysUtils unit. Just means I have to include this code to my source:

function DirectoryExists(const Name: string): Boolean;
var
  Code: Integer;
begin
  Code := GetFileAttributes(PChar(Name));
  Result := (Code <> -1) and (FILE_ATTRIBUTE_DIRECTORY and Code <> 0);
end;

Not a real problem. :-)

I have replaced RaiseLastWin32Error with "raise Exception.Create( SysErrorMessage( GetLastError ) );" and yes, it's another minor bug. First 100 points are gone now. Still 400 to go...

The suggestion for creating the folder if it doesn't exist is nice, but I just think it's better to tell the user the folder just doesn't exist. Basically, I don't like it because the NewFolderChange function already has several parameters (including default ones) and it would only get more complicated if I add more parameters.

I've upgraded the site, btw. These bugs are fixed now. Thanks.
>> The suggestion for creating the folder if it doesn't exist is nice, but I just think it's better to tell the user the folder just doesn't exist. Basically, I don't like it because the NewFolderChange ...

No, this doesn't deserve points, but maybe you can add it to the constructor as parameter with default value, since you already have 2 params with default value...?

NewFolderChange(bla bla bla; AutoCreateFolder: Boolean = True);

and so,

if AutoCreateFolder then
  ForceDirectories(Filter) // from the FileCtrl unit
else if not DirectoryExists(Filter) then
  raise Exception.Create('Duh! Can''t even trust you to select a valid directory!');

Also, Borland's style guide prefers function(value) rather than function( value ); hehehe ;-)
SOLUTION
Link to home
membership
This solution is only available to members.
To access this solution, you must be a member of Experts Exchange.
Start Free Trial
if an exception is raised in the FolderChange event, it shows the message as expected (behind the main window)
but then the application hangs and the CPU utilization goes to 100%...

I haven't debugged the cause of this, and if you control the code and make sure there are no exceptions in the event there are no problems
but I just thought I'd mention it... I'll see if I can figure out how to avoid this behaviour...

I used this code to test:

procedure TForm1.HasChanged(const Path:string);
var
  SearchRec: TSearchRec;
  Source: string;
  Target: string;
  AFile: TStream;
begin
  Memo1.Lines.Add(Format('Files changed in "%s".', [ Path ]));
  if (FindFirst(Path+'*.*', faAnyFile - faDirectory, SearchRec )=0) then
  begin
    repeat
      try
        Source := Path + SearchRec.Name;
        Sleep(0);
        TFileStream.Create(Source, fmOpenReadWrite + fmShareExclusive).Free; // Just to check if we can open the file exclusively.
        DeleteFile(Source);
      except
        on E: Exception do
          Memo1.Lines.Add(E.Message);
      end;
    until (FindNext(SearchRec)<>0);
    FindClose(SearchRec);
  end;
  raise Exception.Create('something wrong'); //raise an exception just for the heck of it... =o)
end;
a dirty "don't care" fix would be to wrap the Notify event in a try..except

function ThreadProc( Value: Pointer ): Integer;
var
  FolderChange: TFolderChange;
  List: array[ 0..1 ] of THandle;
  Finished: Boolean;
  WaitResult: DWORD;
begin
  Result := 0;
  FolderChange := TFolderChange( Value );
  Finished := False;
  List[ 0 ] := FolderChange.FHandle;
  List[ 1 ] := FolderChange.FEvent.Handle;
  repeat
    WaitResult := WaitForMultipleObjects( 2, @List, False, INFINITE );
    if ( WaitResult = WAIT_OBJECT_0 ) then begin
      // We can call the callback routine now.
      try
        FolderChange.DoNotify;
      except //don't care about exceptions???
         //do whatever you want here...
      end;  
you can use the same kind of MessageBox call to display the error I guess, but I don't know if that's desirable...
SOLUTION
Link to home
membership
This solution is only available to members.
To access this solution, you must be a member of Experts Exchange.
Start Free Trial
Wow, quite a few new comments! Cool!

@DragonSlayer:
While the Borland style guide prefers function(value):result; I still prefer to use function( value ): result; but if you want to change it, go visit http://www.dow.wau.nl/aew/DelForExp.html and download this Delphi Formatting Expert. :-)

The AutoCreateFolder could be added with a default value, of course. (Default False in my opinion.) But the question is then at which position it should be in the list of default parameters? I just use default parameters so you can forget about the less common settings. But should AutoCreateFolder be before or after NotifyFilter? Before or after WatchSubtree? The problem is that you can't leave default parameters blank in Delphi thus you would force the user to "guess" the proper value for default values that need to be filled because you want to set another default value...

 Okay, about the TUniqueNumberGenerator, making NewNumber a virtual method isn't very useful since this class is NOT available from outside the unit. All I've made available is an interface. Besides, considering the very small amount of code in this class, it's not even worth the trouble. It would be just as easy to create another class that supports this interface. :-)
Besides, I just wanted a very simple number generator. But I just detected that this number generator isn't thread-safe. :-) I will fix that! (Adds a nice example of how to use the critical section interface.)

@BlackTigerX:
The exception in FolderChange is a good one, though. Basically, this unit doesn't know anything about the rest of the application. Not even about the application or it's main form. This is actually a major bug in my opinion. I forgot to check for exceptions when calling the callback function! Darn, I must think of a NotifyError event too, I guess...
I have just added a second event that will be called if assigned and it will return the error message. If there is no OnError event or the events generates an exception too then tough. I won't show any error in that case but will just continue to run. Not a nice solution but this unit should also be usable in unattended processes and services, so I can't just display a messagebox or whatever since that could cause a lot of troubles. Just imagine a webapplication that is waiting for a user on the server to click on OK...
Messageboxes are definitely undesirable...

@Madshi:
LOL. Never even noticed the similarities, though. I don't have the sourcecode of your components so I never noticed it even. Your version seems a bit more complicated, though. One difference though. I don't pass any security attributes. And the parameters I've used are closer to the names used by the API itself. O don't have an OpenXXX version for these objects either but will just tell you with the Existing property if a new object was created (false) or if you're linked to an existing (true) object.  I considered this a nicer solution.
And yes, I love interfaces too. (I just noticed I forgot to add GUIDs to two of the interfaces too.) I especially like them because you don't have to worry too much about freeing them, once you're done with them. So easy to pass them along from one function to the other without worrying where you should call the Free method. Also quite useful if you put them in a DLL and share them between DLL and executable or even between multiple DLL's. (Although in this case you do have to be aware of memory allocations!)
And you have a nicer WaitFor method in case someone wants to use for multiple objects. I just kept it simple and implemented a WaitForSingleObject with a time-out. But since all interfaces expose the handle of the object, you can always call WaitForMultipleObjects with an array of these handles.

Score so far:
DragonSlayer: 50 points + 50 points.
BlackTigerX: 100 points.
Madshi: None yet. ;-)

Still 300 points to be divided... :-)

(Am also thinking about which other API thingie that could use a simple interface wrapper.)
for the default parameter, I would put it at the end, when adding default parameters I always add them to the end, since that allows for backwards compatible code, and if I have new code that is going to use those, then the new code has to supply all the other parameters as well, but the old code can still use the same function

so you are going to add an event for OnError, that's a good idea, just don't forget to (again) wrap the OnError with try..except, in case someone decides to generate an exception on the OnError event, I know it may sound stupid, but you just never know... if there is no OnError defined you could create an application event that gets logged in the Windows events, that would be nice but I don't know if necessary... mmm... no... if they didn't define an OnError event, is because they don't care about the error, so that's all you need to do

"I have replaced RaiseLastWin32Error with "raise Exception.Create( SysErrorMessage( GetLastError ) );" and yes, it's another minor bug. First 100 points are gone now. Still 400 to go..."

I thought we only had 400 left... =oP
It's a 500 points question but can only divide them once there are enough bugs found. Thus a serious encouragement to go hunting for those big ones. :-P
Still some 300 points to go now, that is 3 major bugs, 6 minor bugs or 15 good suggestions. ;-)

However, making interesting suggestions is also worth points. Okay, only 20 each but hey... You help improve some code that is shared with the whole EE community. (Don't think anyone will consider it very useful, though. It's quite technical stuff.)

Did you refresh the sourcecode page, btw? Then you can see that I already thought of this exception handler on the OnError event. :-) If even this crashes then it just loses all hope and just waits for the next notification...
If there's no way to report the error to the system then I want my code to just continue running. Under no condition should it stop or quit processing... Thus, no messageboxes and preferably no unhandled exceptions...

For backwards compatibility, I might want to overload some of those NewXXX methods instead of adding parameters. Actually, checking if a folder exists could be done from the NewFolderChange instead. Then you'd create the folder even before the object is created. That's the interesting part of these solutions. You can add more methods that will create these objects for you, but which might have even more parameters.
you fixed this:

else begin
      // Some error occurred. Finish anyway.
      FolderChange.DoOnError( SysErrorMessage( GetLastError ) );
      Finished := True;
    end;

but haven't fixed this:

      // We can call the callback routine now.
      try
        FolderChange.DoNotify;
      except
         //call OnError event here
      end;  


overloading the method sounds like a better option...
@BlackTigerX,
That's done on purpose! Basically, I tell the thread to finish when WaitForMultipleObjects returns an unexpected result. (E.g. because it has an invalid handle.) It cannot time-out because it's set to infinite, thus the only results I expect are WAIT_OBJECT_0 or WAIT_OBJECT_0+1. When I get WAIT_OBJECT_0+1 then I know the thread must terminate since an event is triggered in the destructor telling it to end. If it's WAIT_OBJECT_0 then I just execute a user-denined action and if something goes wrong in it then that's not something for this object to solve. If the user is too lazy to do some exception handling in his own code then I will just ignore any error.
Of course I could just terminate the thread but then the system won't get any further notices. But I choose to keep the system running and just ignore any exception that occurs in the user-defined code. I think these kinds of errors are up to the user to catch, although I will report any unhandled exceptions in the OnNotify event.

There is of course the problem that even when the thread is freed, the object is still not freed itself! I can't even free the object since there's some variable pointing to it, which would result in funny behaviour if the object is suddenly freed. And no, I don't have any way to tell where this variable actually is. Thus, once the thread stops running, the object just becomes inactive, which you can check with the IsRunning property.

Keep in mind that I want this code to run in all kinds of different situations. Not just a simple GUI application but also in console applications, system services, control applets, screensavers and perhaps even in webserver applications. This means the code has to be very lightweight and independant, but above all... If possible, it should continue to run until it is freed in a clean way.

Btw... The exception handling around the user-defined code is in the functions DoNotify and DoOnError methods themselves. ;-) No need to catch them anywhere else.
Okay, time to close this question. :-)

For those who are interested, this unit continues to be work in progress but the most important things seem to work now. It now includes some trick to check for duplicate string values. Basically, this is a write-only list type (based on a hash table) that you can add string values to. If a value has already been added, it will fail to add it again, thus you know it's a duplicate. I've used it in the past to check a file of half a million lines for duplicate values and it did a good job in about half a minute. :-)

It also supports a simple hashtable type that allows you to write objects to a binary file. These objects must be indexed by an unique string value but the purpose of this table is to store a huge amount of objects and still allow fast access to them. This is work in progress, though, since I now need code to delete objects from this table too and a way to pack them. This object is a bit based upon how the old TurboVision (Remember Turbo Pascal) works.

And some simple interface to access system services, although I want to redesign this completely now...

And also included: an interesting trick to redirect write/writeln commands to output whatever you write to them to an object. Two "examples" in the code show how it works. (The GetDebugFile and GetLogFile functions combined with the AssignInterface procedure.)

So now, time for points...

Responses from 3 persons: BlackTigerX, DragonSlayer and Madshi. 500 points to divide. Number of bugs found: 2 minor, 1 major. The bugs were:
BlackTigerX: The missing check to see if a folder even exists. (Minor)
BlackTigerX: RaiseLastWin32Error is deprecated. (Minor)
BlackTigerX: Exception in the FolderChange event results in CPU utilization of 100%. (Major)

Why did I think DragonSlayer suggested the two minor bugs???

Now, about suggestions... Madshi's componentsare quite interesting and show that I'm not the only one who loves to use interfaces. DragonSlayer suggested a minor modification for the unique number generator but since the class itself is never exposed to the outside world I don't include it. Still, most credits are for BlackTigerX, so the division will be:
BlackTigerX: 420 points.
DragonSlayer and Madshi each 40 points.
LoL... I got points too ;-)
Yep. It seemed reasonable to me to reward everyone who at least participated to finding these bugs, even if it was a simple comment. :-)

What matters to me is that some experts tell me "I have looked at it and didn't see anything wrong with it." :-P