Задача, прошу review решения

Question

[datalink]

Есть вот такая задачка, название конторы-автора оставим за рамками данного обсуждения. Перед тем как постить полный текст, я сам поискал несколько фраз из него в нете и, как ни странно, нашел. Эта публикация не станет первой. Итак…

Используя С++, Win32 API и STL корректно реализовать следующую задачу:

Откуда-то дано:

      class Request
      {
      };

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

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

Задача:

1) Организовать приём запросов, для этого класть в одну очередь задания, возвращаемые функцией GetRequest.
2) Запустить несколько обрабатывающих запросы потоков (переменное число, но не менее двух), которые должны обрабатывать поступающие из очереди задания с помощью ProcessRequest.
3) Поработать в течение 30 секунд.
4) Корректно остановить все потоки. Если остались необработанные задания, не обрабатывать их и корректно удалить.
5) Завершить программу.

Тип Stopper должен быть определён вами и должен представлять собой механизм досрочной остановки выполняемого действия (предполагается, что GetRequest и ProcessRequest будут его корректно использовать).
Вызов GetRequest может не сразу возвращать задания.
Вызов ProcessRequest может не мгновенно обрабатывать задание.
— Нарисовал за вечер, отправил, ответ получил лишь через неделю, в духе «решение не понравилось, извините», без конкретики. Просьба к уважаемому хабрасообществу пояснить возможные косяки. По ссылке ~300 строк кода.

Собственно мой вариант: http://pastebin.com/kTQDQyLr

Интересует, что именно плохо и почему так делать нельзя. Исключительно чтобы впредь таких ошибок не совершать.
Указывайте в комментариях суть проблемы, название класса\метода\номера строк. Заранее спасибо!

Comments

[datalink]

Благодарю откликнувшихся, познавательно!

Answer 1

[m08pvv]

Первое, что бросилось в глаза — банальная ошибка копипасты:

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

Comments

[m08pvv]

Во-вторых производитель бездумно забивает очередь и если в ней накопится много заданий, то приложение упадёт (банально памяти не хватит) — как минимум надо ограничить количество элементов в очереди.

Answer 2

[mejedi]

Есть недочеты как в части знания и применения идиом C++, так и в скиле системного программирования.

Базовый класс InterthreadObject — фе. В данном случае правильно использовать агрегацию. Вместо Enter/LeaveCriticalSection использовать класс lock a-la boost scoped_lock. Что на счет копирования/присваивания таких объектов?

StopperCondition — а зачем там вобще синхронизация? (Здесь соискатель может начинать рассказывать про volatile, барьеры и тд).

InterthreadRequestQueue перевести на умные указатели или обосновать почему они здесь не нужны. Не использовать new[] для выделения буфера под хэндлы. Сами хэндлы сделать объектом для автоматического закрытия в деструкторе.

Возможно им не понравился printf. Лично у меня вызывает неприятие getc(cin) в конце main. Возможно, ожидается обработка исключений раз уж GetRequest/StopRequest старательно объявлены как no-throw.

Consumer thread будет находиться в состоянии активного ожидания при исчерпании очереди. Более того несколько консъюмеров будут мешать друг другу и продьюсеру постоянно борясь за одну критическую секцию. Я бы оснастил очередь condition variable, это бы решило данную проблему.

Answer 3

[vScherba]

Никогда не используйте CreateThread в C/C++ приложениях, используйте _beginthreadex. Об этом написано в каждом учебнике по WinAPI и в самой MSDN. Это сразу не понравится ревьюверу.
Конечно, можно в комментариях обосновать, что CreateThread использован специально, что в потоковой функции не используются вызовы CRT, но все равно, смысла в этом мало, потоковая функция может в будущем измениться.