Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Added code to make pointer vector be thread safe #1325

Closed
wants to merge 11 commits into from
67 changes: 58 additions & 9 deletions Common++/header/PointerVector.h
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
#include <stdio.h>
#include <stdint.h>
#include <vector>
#include <mutex>

/// @file

Expand All @@ -13,13 +14,23 @@
namespace pcpp
{

/**
* @struct nullMutex
* A struct containing standard mutex operation but does not perform any action.
* This mutex is used when the user decides PointerVector not be thread safe which be default
*/
struct nullMutex {
muthusaravanan3186 marked this conversation as resolved.
Show resolved Hide resolved
void lock() const {}
void unlock() const {}
};

/**
* @class PointerVector
* A template class for representing a std::vector of pointers. Once (a pointer to) an element is added to this vector,
* the element responsibility moves to the vector, meaning the PointerVector will free the object once it's removed from the vector
* This class wraps std::vector and adds the capability of freeing objects once they're removed from it
*/
template<typename T>
template<typename T, typename Mutex=pcpp::nullMutex>
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please update the document.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please rename it to NullMutex for consistency in this project. Also, maybe it's better to put NullMutex in detail namespace, since it is only for internal usage.

namespace detail
{
	struct NullMutex { ... }
} // namespace detail

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

changed to NullMutex. But detail namespace is only present on Pcap++. Since this on Common folder hoping will place it here which is more reasonable.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@tigercosmos where we need to update the document?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can update the comment, mentioning about the template argument. We use Doxygen to generate the document.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

class PointerVector
{
public:
Expand All @@ -43,6 +54,7 @@ namespace pcpp
*/
~PointerVector()
{
std::lock_guard<Mutex> lk(m_Mutex);
for (auto iter : m_Vector)
{
delete iter;
Expand All @@ -55,6 +67,7 @@ namespace pcpp
*/
PointerVector(const PointerVector& other)
{
std::lock_guard<Mutex> lk(m_Mutex);
muthusaravanan3186 marked this conversation as resolved.
Show resolved Hide resolved
for (const auto iter : other)
{
tigercosmos marked this conversation as resolved.
Show resolved Hide resolved
T* objCopy = new T(*iter);
Expand All @@ -67,6 +80,7 @@ namespace pcpp
*/
void clear()
{
std::lock_guard<Mutex> lk(m_Mutex);
for (auto iter : m_Vector)
{
delete iter;
Expand All @@ -78,31 +92,51 @@ namespace pcpp
/**
* Add a new (pointer to an) element to the vector
*/
void pushBack(T* element) { m_Vector.push_back(element); }
void pushBack(T* element)
{
std::lock_guard<Mutex> lk(m_Mutex);
m_Vector.push_back(element);
}

/**
* Get the first element of the vector
* @return An iterator object pointing to the first element of the vector
*/
VectorIterator begin() { return m_Vector.begin(); }
VectorIterator begin()
{
std::lock_guard<Mutex> lk(m_Mutex);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this protects as much as you would think.
Yes, you don't get data races when obtaining the iterator but vector iterators generally modify the vector memory on their own, so after the safety car is gone (begin iterator is obtained), all the threads are free to race again via direct iterator modification.

Copy link
Author

@muthusaravanan3186 muthusaravanan3186 Mar 1, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the case on normal scenario too even without multi threading environment. we get an iterator and after some time vector reconstructs itself then the above iterator will be undefined.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

True, tho in a non-multithreaded scenario you don't have the option of another thread invalidating your iterator whenever. Iterators are invalidated in specific documented calls and the sequence of those calls in relation to iterator usage is deterministic.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we are planned to use shared_ptr in another PR

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps we want to add this line (and all other similar ones) later when you raise another PR.

return m_Vector.begin();
}

/**
* Get the first element of a constant vector
* @return A const iterator object pointing to the first element of the vector
*/
ConstVectorIterator begin() const { return m_Vector.begin(); }
ConstVectorIterator begin() const
{
std::lock_guard<Mutex> lk(m_Mutex);
muthusaravanan3186 marked this conversation as resolved.
Show resolved Hide resolved
return m_Vector.begin();
}

/**
* Get the last element of the vector
* @return An iterator object pointing to the last element of the vector
*/
VectorIterator end() { return m_Vector.end(); }
VectorIterator end()
{
std::lock_guard<Mutex> lk(m_Mutex);
muthusaravanan3186 marked this conversation as resolved.
Show resolved Hide resolved
return m_Vector.end();
}

/**
* Get the last element of a constant vector
* @return A const iterator object pointing to the last element of the vector
*/
ConstVectorIterator end() const { return m_Vector.end(); }
ConstVectorIterator end() const
{
std::lock_guard<Mutex> lk(m_Mutex);
muthusaravanan3186 marked this conversation as resolved.
Show resolved Hide resolved
return m_Vector.end();
}


//inline size_t size() { return m_Vector.size(); }
Expand All @@ -111,13 +145,21 @@ namespace pcpp
* Get number of elements in the vector
* @return The number of elements in the vector
*/
size_t size() const { return m_Vector.size(); }
size_t size() const
{
std::lock_guard<Mutex> lk(m_Mutex);
return m_Vector.size();
}

/**
* Returns a pointer of the first element in the vector
* @return A pointer of the first element in the vector
*/
T* front() { return m_Vector.front(); }
T* front()
{
std::lock_guard<Mutex> lk(m_Mutex);
return m_Vector.front();
muthusaravanan3186 marked this conversation as resolved.
Show resolved Hide resolved
}

/**
* Removes from the vector a single element (position). Once the element is erased, it's also freed
Expand All @@ -126,6 +168,7 @@ namespace pcpp
*/
VectorIterator erase(VectorIterator position)
{
std::lock_guard<Mutex> lk(m_Mutex);
delete (*position);
return m_Vector.erase(position);
}
Expand All @@ -139,7 +182,10 @@ namespace pcpp
{
T* result = (*position);
VectorIterator tempPos = position;
tempPos = m_Vector.erase(tempPos);
{
std::lock_guard<Mutex> lk(m_Mutex);
tempPos = m_Vector.erase(tempPos);
muthusaravanan3186 marked this conversation as resolved.
Show resolved Hide resolved
}
position = tempPos;
return result;
}
Expand All @@ -151,6 +197,7 @@ namespace pcpp
*/
T* at(int index)
{
std::lock_guard<Mutex> lk(m_Mutex);
return m_Vector.at(index);
muthusaravanan3186 marked this conversation as resolved.
Show resolved Hide resolved
}

Expand All @@ -161,11 +208,13 @@ namespace pcpp
*/
const T* at(int index) const
{
std::lock_guard<Mutex> lk(m_Mutex);
return m_Vector.at(index);
muthusaravanan3186 marked this conversation as resolved.
Show resolved Hide resolved
}

private:
std::vector<T*> m_Vector;
mutable Mutex m_Mutex;
};

} // namespace pcpp
Loading