Критический раздел - быть или не быть? - PullRequest
Новая
       14

Критический раздел - быть или не быть?

2 голосов
/ 26 декабря 2008

Я пишу чат, используя функции WinSock2 и WinAPI. И у меня небольшая проблема.
Я храню std :: vector клиентских подключений на сервере. Когда подключается новый клиент, запускается новый поток, и вся работа с клиентом выполняется в этом новом потоке. Я не использую классы (я знаю, что это не очень хорошо), поэтому этот список соединений просто определен как глобальная переменная.
Мне кажется, что это может быть ситуация, когда несколько потоков пытаются получить доступ к этому списку одновременно. Хотя я не заметил, что есть какие-то проблемы с этим, мне нужно сделать что-то вроде этого: 


template 
class SharedVector {
    std::vector vect;
    CRITICAL_SECTION cs;
    SharedVector(const SharedVector& rhs) {}
public:
    SharedVector();
    explicit SharedVector(const CRITICAL_SECTION& CS);
    void PushBack(const T& value);
    void PopBack();
    unsigned int size();
    T& operator[](int index);
    virtual ~SharedVector();
};

template
SharedVector::SharedVector() {
    InitializeCriticalSection(&cs);
}

template
SharedVector::SharedVector(const CRITICAL_SECTION& r): cs(r) {
    InitializeCriticalSection(&cs);
}

template
void SharedVector::PushBack(const T& value) {
    EnterCriticalSection(&cs);
    vect.push_back(value);
    LeaveCriticalSection(&cs);
}

template
void SharedVector::PopBack() {
    EnterCriticalSection(&cs);
    vect.pop_back();
    LeaveCriticalSection(&cs);
}

Итак, требует ли моя ситуация использования CRITICAL_SECTION, и я просто счастливчик, который не нашел ошибку?

Ответы [ 5 ]

7 голосов
/ 26 декабря 2008

Да, вам просто повезло, что у вас никогда не было проблем. Эта проблема связана с проблемами синхронизации и условиями гонки: код будет работать в 99,9% случаев, а при возникновении аварии вы не будете знать, почему.

Я бы забрал конструктор, взяв CRITICAL_SECTION в качестве параметра, так как без рассмотрения (предположительно несуществующей) документации невозможно понять, что конструктор его инициализирует.

3 голосов
/ 08 января 2009

Самый большой вопрос, который у меня к вам, - архитектурный. Действительно ли поток для каждого соединения нуждается в прямом доступе к этому массиву других соединений? Разве они не должны ставить в очередь какие-то абстрактные сообщения? Если каждый поток соединения осознает детали реализации юниверса, в котором он живет, я ожидаю, что у вас возникнут проблемы где-то в будущем.

Но давайте предположим, что потокам соединения действительно нужен прямой доступ друг к другу ...

Глобальные переменные по своей природе не являются злом; школы просто учат этому, потому что это проще, чем предоставлять детальное понимание. Вы должны точно знать, что такое глобальная переменная, и приличное эмпирическое правило полагать, что глобальный - это неправильный выбор, пока вы не обнаружите, что у вас нет альтернативы. Одна альтернатива, которая решает ряд проблем, заключается в написании такой функции: 

SharedVector & GetSharedVector (void)
{
    static SharedVector sharedVector;
    return sharedVector;
}

Это дает вам больше контроля над тем, когда создается экземпляр класса, чем с глобальной переменной, что может быть критично, если у вас есть зависимости среди глобальных переменных, которые имеют конструкторы, особенно если эти глобальные переменные порождают потоки. Это также дает вам возможность вступать в конфликт, когда кто-то хочет получить доступ к этой «глобальной» переменной, а не просто бессильно страдать, пока они прямо тыкают в нее.

Ряд других мыслей также приходит на ум. Без определенного порядка ...

  • Вы, несомненно, уже знаете Синтаксис шаблона, который вы используете здесь странно; Я предполагаю, что вы только что набрали этот код без компиляции.
  • Вам нужно сделать ничего не делать оператор в вашем личном разделе предотвратить несчастные случаи (для того же причина, у вас есть ничего не делать копию конструктор есть).
  • Явный конструктор копирования сломано несколькими способами. Нужно явно скопировать вектор или вы получите пустой вектор в вашем новом экземпляре SharedVector. И он должен просто инициализировать его собственный критический раздел, а не скопируйте это (и затем инициализируйте это). Действительно для твоего случая это наверное не имеет смысла иметь это функционировать вообще, потому что сеть соединения не имеют разумного копировать семантику.
  • Вы можете упростить безопасность исключений создавая класс EnteredCriticalSection, чья вызов конструктора EnterCriticalSection и чей вызовы деструкторов LeaveCriticalSection. (Нужно держаться за ссылку на критический раздел, конечно.) Это облегчает безопасную работу сериализовать вашего другого члена функции перед лицом исключений.
3 голосов
/ 26 декабря 2008

Этот код не является безопасным для исключений, векторные методы push_back и pop_back могут генерировать исключения, и здесь у вас есть потенциальная тупиковая ситуация. Это методы: 

unsigned int size();
T& operator[](int index);

также должны быть синхронизированы, поскольку они имеют доступ к данным, которые могут быть изменены другим потоком. Например, вы можете получить доступ к данным следующим образом: 

value = shared_vector[shared_vector.size() - 1];

и в то же время другой поток может сделать это: 

shared_vector.PopBack();
1 голос
/ 26 декабря 2008

Да, если вы выставляете глобальный вектор, подобный этому, вам обязательно нужно заблокировать его при любом чтении / записи. И да, это может сильно повредить вашей производительности.

Кроме того, один новый поток на запрос обычно тоже не очень хорошая идея. Почему бы вам не перепроектировать ваше приложение, чтобы вместо него использовать порты завершения ввода-вывода?

0 голосов
/ 26 декабря 2008

И, кстати, это плохой способ объявить этот вектор глобальной переменной. Будет ли лучше сделать его локальным?

...