Re: Thread Pre-Emption Question . . .

See Inline:

"Chris Thomasson" <cristom@xxxxxxxxxxx> wrote in message
news:F_ydnSkVe6qdjKranZ2dnUVZ_uqvnZ2d@xxxxxxxxxxxxxx
"m" <m@xxx> wrote in message news:udKFfc9IIHA.484@xxxxxxxxxxxxxxxxxxxxxxx
I am not sure why you are so defensive about your code - I haven't
attacked it at all. I haven't said anything about it at all except that I
hadn't reviewed it in detail and that at first glance it looked odd.
[...]
All of that being said, you code still looks odd and, based on my
analysis, making assumptions about how the code is used, has both bugs
and a high probability of correct execution. Specific bugs:



1) Not checking for failure of InitializeCriticalSection,

You can't really check if InitializeCriticalSection fails because it
returns void... However, you can use InitializeCriticalSectionAndSpinCount
to make sure that everything is correctly initialized at constructor time.

Of course! - but the choice of the wrong API is still a bug ;) While not
strictly a bug, this API will raise STATUS_NO_MEMORY and he'll need an SEH
handler to catch it and it will be very difficult to construct to fail
gracefully.

CreateEvent or CreateSemaphore in the constructors. These are C APIs
that do not raise exceptions on failure so their return codes must be
checked and an exception thrown on failure to abort the constructor.
BTW: not checking deallocateor failure is okay because the program will
continue to run correctly although it is good practice to check this in
debug builds because deallocators usually only fail because of invalid
arguments.

All correct indeed. However, not only does he fail to check any return
values in the constructor, he fails to check any of them within in the
class body... For instance, he make numerous calls to SetEvent,
ResetEvent, ReleaseSemaphore and WaitForSingleObject without checking for
any errors. I hope that he corrects these issues because its a very bad
habit to get into simply because the end result is incorrect code.


As Alexander said: we know, as a detail of this implementation, that WFSO,
SetEvent, etc. don't fail unless an invalid handle is passed. This means
that after init, this code will run successfully on all versions of Windows.
And while relying on implementation details is _bad_, what are you
reasonably going to do if you call to SetEvent fails? About the only think
you can do is to ABEND. This is a sanity check that really should be
present in top quality code, especially debug builds, but won?t ever be
executed unless there are other bugs in the code.


Specific performance issues:



1) ReaderEnter need not grab WriterMutex and could be much more
efficient if it employed an optimistic locking strategy (assume that the
lock state is consistent with desired access until it is discovered
otherwise) using InterlockedIncrement & InterlockedDecrement and a loop

2) In WriterEnter, ResetEvent need not be called every time. It
will be a NOOP if WriterRecursion is >= 1.



Before you start at me and say again that this has run for years and
years with no problem, let me remind you that the bugs I've identified
have a very low probability of occurrence (should only occur during a low
memory condition) and only exist in the constructors so the number of
calls is few.

True.




And depending on what you do while holding this lock may not prevent the
correct execution of error handling logic (if the access causes unplanned
exceptions before data corruption and these unplanned exceptions are
caught in the same place as planned exceptions for example).

Also true. The most dangerous bug I found in his code is in the following
snippets:


_____________________________________________________________
inline void TReaderWriter::ReaderEnter()
{
TCriticalSectionGrabber grab(WriterMutex);
WaitForSingleObject(ReadingOk, INFINITE);
if (InterlockedIncrement(&Readers) == 0) {
WaitForSingleObject(ReadingActiveSemaphore, INFINITE);
}
}

inline void TReaderWriter::WriterEnter()
{
WriterMutex.Enter();
WriterRecursion++;
ResetEvent(ReadingOk);
if (WriterRecursion == 1) {
WaitForSingleObject(ReadingActiveSemaphore, INFINITE);
}
}
_____________________________________________________________


If one of those WaitForSingleObject calls just happens to fail for any
reason whatsoever, the code will simply enter the write and/or read
critical section when its clearly not suppose to. I say and/or because
this could allow a reader and writer to access the data concurrently. This
can, and will, corrupt real data in any number of random ways depending on
the work done in the writer. This means that if you use this code to
protect some of your data, well, its like playing Russian Roulette with a
double barreled shotgun.

I have asked "Pops" about this issue several times and he refuses to
answer. I sure do hope that he decides to all of the bugs in his code, for
his customers sake. I don't know why he starts to shout and get angry, as
we are only trying to help him by pointing out the obvious mistakes.

He should be thanking us.


[...]




As for the advise you gave to the OP, well, your correct... The OP simply
cannot rely on processing to complete within a 40ms window. The OP must
realize that he will experience random failures due to the processing
taking longer than 40ms, period.


.

Relevant Pages

  • Re: Thread Pre-Emption Question . . .
    ... Not checking for failure of InitializeCriticalSection, ... not checking deallocateor failure is okay because the program will continue to run correctly although it is good practice to check this in debug builds because deallocators usually only fail because of invalid arguments. ... the correct execution of error handling logic (if the access causes unplanned exceptions before data corruption and these unplanned exceptions are caught in the same place as planned exceptions for example). ... This can, and will, corrupt real data in any number of random ways depending on the work done in the writer. ...
    (microsoft.public.win32.programmer.kernel)
  • Re: Thread Pre-Emption Question . . .
    ... Not checking for failure of InitializeCriticalSection, ... not checking deallocateor failure is okay because the program will continue to run correctly although it is good practice to check this in debug builds because deallocators usually only fail because of invalid arguments. ... correct execution of error handling logic (if the access causes unplanned exceptions before data corruption and these unplanned exceptions are caught in the same place as planned exceptions for example). ... This can, and will, corrupt real data in any number of random ways depending on the work done in the writer. ...
    (microsoft.public.win32.programmer.kernel)
  • Re: required attribute "ALT" not specified .
    ... what is the simple logic I fail to ... "iMacs are good" is wrong. ... be uncategorically hypocritical to make personal exceptions. ... I take everything you say with the utmost seriousness. ...
    (alt.html)
  • RE: Execute failed: ORA-01026: multiple buffers of size > 4000 in the bind list (DBD: oexec erro
    ... > incorrect NLS_LANG environment variable settings, or bugs. ... jwm wrote. ... The program did not fail. ... I have implemented the following work around and have not had a failure ...
    (perl.dbi.users)
  • Re: Execute failed: ORA-01026: multiple buffers of size > 4000 in the bind list (DBD: oexec erro
    ... >> incorrect NLS_LANG environment variable settings, or bugs. ... > Tim. ... I took the print out and program would fail. ... > I have implemented the following work around and have not had a failure ...
    (perl.dbi.users)