Task, please review the solution

Question

Multithreading

There is such a problem, we will leave the name of the author's office outside the scope of this discussion. Before posting the full text, I myself looked for a few phrases from it on the net and, oddly enough, I found it. This post will not be the first. So…

Using C++, Win32 API and STL correctly implement the following task:

Given from somewhere:

      class Request
      {
      };

      // возвращает NULL, если объект stopSignal указывает на необходимость остановки,
      // либо указатель на память, которую в дальнейшем требуется удалить
      Request* GetRequest(Stopper stopSignal) throw(); 

      // обрабатывает запрос, но память не удаляет, завершает обработку досрочно, если
      // объект stopSignal указывает на необходимость остановки
      void ProcessRequest(Request* request, Stopper stopSignal) throw();

Task:

1) Organize the reception of requests, for this put the tasks returned by the GetRequest function in one queue.
2) Start several request processing threads (a variable number, but not less than two), which should process incoming jobs from the queue using ProcessRequest.
3) Work for 30 seconds.
4) Correctly stop all threads. If there are unprocessed tasks left, do not process them and delete them correctly.
5) End the program.

The Stopper type must be defined by you and must be a mechanism for stopping the action in progress early (GetRequest and ProcessRequest are expected to use it correctly).
The call to GetRequest may not immediately return jobs.
The call to ProcessRequest may not immediately process the job.
- I drew it in an evening, sent it, I received an answer only a week later, in the spirit of “I didn’t like the solution, sorry,” without specifics. Request to the respected habra community to explain possible jambs. According to the link ~ 300 lines of code.

Actually my version: http://pastebin.com/kTQDQyLr

Interested in what exactly is bad and why it can not be done. Just to avoid making such mistakes in the future.
Indicate the essence of the problem, the name of the class\method\line numbers in the comments. Thanks in advance!

Answer 1

The first thing that caught my eye was a banal copy-paste error:

hSomeConsumerThread = CreateThread( NULL, 0, consumerThread, &s, 0, NULL);
if (NULL == hProducerThread)
{

Answer 2

There are shortcomings both in terms of knowledge and application of C ++ idioms, and in the skill of system programming.
The base class of InterthreadObject is fe. In this case, it is correct to use aggregation. Instead of Enter/LeaveCriticalSection use class lock a-la boost scoped_lock. What about copying/assigning such objects?
StopperCondition - why is there just synchronization? (Here the applicant can start talking about volatile, barriers, etc.).
InterthreadRequestQueue translate to smart pointers or justify why they are not needed here. Do not use new[] to allocate a buffer for handles. Make the handles themselves an object for automatic closing in the destructor.
Maybe they didn't like printf. Personally, I hate getc(cin) at the end of main. Perhaps exception handling is expected since GetRequest/StopRequest are carefully declared as no-throw.
The consumer thread will be in an active wait state when the queue is exhausted. Moreover, several consumers will interfere with each other and the producer by constantly fighting for one critical section. I would equip the queue with a condition variable, that would solve this problem.

Answer 3

Never use CreateThread in C/C++ applications, use _beginthreadex. This is written in every WinAPI tutorial and in MSDN itself. The reviewer will not like it right away.
Of course, you can justify in the comments that CreateThread was used on purpose, that CRT calls are not used in the thread function, but anyway, this makes little sense, the thread function may change in the future.