I got caught today by a subtle bug in my current project and ended up wasting about 8 hours tearing my hair out. Read on for the low down on how I dealt with copying classes that manage a unique resource, in this case a find handle.

I'm writing an STL compatible iterator for traversing the files in a directory. I'm on Win32 so I'm using the standard FindFirstFile/FindNextFile/FindClose API sequence.

The file iterator takes a directory path in one of its constructors. The constructor uses this to call FindFirstFile. The return value is a find handle to be used in subsequent searches. Operator++ calles FindNextFile using the remembered handle. The destructor calls FindClose, cleaning up after the iterator and freeing valuable windows resources.

It took about 45 minutes to write the class including the corresponding suite of CppUnit unit tests. All was tickety-boo, the unit tests ran at 100%, so I moved on to extend the class that was actually going to use the iterator. The goal was to implement a begin() method that would initialise a file iterator with the appropriate directory and return it for use by a client.

But how best to write the method? Should it return a reference to an iterator, a pointer or a copy? I didn't want to have to rely on the client doing any cleanup - that's the path to disaster. Passing back by reference seemed the most efficient and safest but that meant creating the iterator on the heap, not the stack, i.e. as a member variable or as a new pointer. Thinking further through it I realised that I'd need multiple instances of the iterator to be independent of one another, requiring some extensive management of heap allocated iterators in the server class. Worse still any cleanup would have to be done in the destructor of the server class which was likely to be a very long-lived object, usually as long as the application was running. This meant that find handles would be held open for the entire duration of the app. The best solution would be to pass back a stack-based copy which would be destroyed automatically when it went out of scope in the client code.

Passing back a stack copy is easy:

return FileIterator(directory);

I coded this up and wrote my unit tests for the new method. Whoops! Two of the tests, the ones that incremented the iterator, threw unknown exceptions! Perhaps the directory was empty? No. I commented out the increments and ran the tests again: no exception. It was definately the increments that were the problem. I stepped through the code and traced the exception to the call to FindNextFile.

I read the API documentation again - no clues. I checked the value of the find handle - it was ok. I spent the next few hours back and forth trying idea after idea to no avail. After much hair pulling I ripped it back to basics and created the iterator object directly in the test case. Surprise! It ran perfectly. So the cause of the exception had to be in the way the iterator was passed back to the client. I looked long and hard and then it finally struck me!

The code I had written (that single line above) doesn't pass that created object back to the client. The compiler actually creates a temporary object on the stack and the client code then copies that value into the local variable. The temporary object is then destroyed. (In fact many compilers do assign the stack copy to the local variable directly as an optimisation which would have made my bug hunt much much harder)

When the temporary object was being destroyed it was calling FindClose and invalidating the find handle. The copy of the object was then merrily using it oblivious to the fact that windows had just reclaimed it!

If I'd been sensible and set the handle to a null value after calling FindClose then I might have spotted this bug earlier. It's the same principle as setting pointers to NULL after deleting them.

I had two options: share the find handle or create a unique one for each instance. For sharing to work somehow I had to guarantee that FindClose was called only after the last object referencing it had finished. That was a tall order, plus there was the possibility of indeterminate behaviour when different iterator copies increment the file pointer independently. So, sharing was ruled out.

What was left was guaranteeing a unique handle for each instance. I could do this in the copy constructor, basically open up a new find handle each time the object is copied. Except that it seemed ineffecient for my use case: passing back by value. It meant that every access to that method would call FindFirstFile twice. Also taking a copy of the object would not copy the state of the iterator, instead it would reset the find handle back to the first file.

So, finally the solution I've finally implemented is that copying the iterator transfers ownership of the find handle to the copy. The original object has its handle invalidated so subsequent find operations will. In effect it is disabled. In this way I can guarantee that the find handle is only ever accessed by a single object and that FindClose is called only once per handle. It works for me and shows how a unique resource can be managed simply. I wonder, though, whether there are any other ways of doing it. Any suggestions from out there?