StorageManagerIF thoughts #68

New Issue

muellerr · 2020-05-07T13:28:36+02:00

muellerr commented

2020-05-07 13:28:36 +02:00

The Pool Manager implements thread-safe access to functions like reserveSpace / deleteData, the read functions are not protected yet from what I have seen.

Right now, the read functions take pointers and set them to the respective addresses in the local pool.
I've wondered what happens if the read function is called and then the data is changed by another thread/task before doing something with that data. I guess the read functions are not really thread-safe but maybe I have forgotten some details..

Anyway, a thread-safe implementation of a read call could be implemented like the other PoolManager functions if a copy is performed. I think this would be a nice feature anyway and it would make usage of the pool more explicit. I passed pointers to an array to the getData() function multiple times and wondered why no data was written to the array. A getDataCopy() function would be nice to get a copy of the data in the local pool (with a lock protection for the pool manager override), a retrieve() function would also be nice to get a copy and then delete the data (again with lock protection for the pool manager override).

Another way would be to lock the pool in the getData() and modifyData() calls for the pool manager by creating a mutex helper on the heap and return a std::unique_ptr<MutexHelper> mutexLock to that mutex helper object to the caller. then the caller has access to the locked datapool and is responsible for unlocking the datapool, or unlocking happens automatically when the pointer gets out of scope. Move semantics allow the transfer the ownership of the unique_ptr to the caller.

A more simple option would be to forbid modifyData() in the StorageManager.
UPDATE: I declared the overriden modifyData() private and code is still compiling without issues.

On another note (maybe sth for another issue):

Usually, a message is sent with a storeId to the data in the pool, which is then expected to be deleted. It's easy to forget a deletion operation. If we send around a std::shared_ptr instead which is tied to a StorageHelper on the heap containing the store address which calls deleteData() automatically on destruction, the deletion would be performed automatically (RAII!). It's a little bit more overhead for a higher safety which does not depend on a developer forgetting something.

The Pool Manager implements thread-safe access to functions like reserveSpace / deleteData, the read functions are not protected yet from what I have seen. Right now, the read functions take pointers and set them to the respective addresses in the local pool. I've wondered what happens if the read function is called and then the data is changed by another thread/task before doing something with that data. I guess the read functions are not really thread-safe but maybe I have forgotten some details.. Anyway, a thread-safe implementation of a read call could be implemented like the other PoolManager functions if a copy is performed. I think this would be a nice feature anyway and it would make usage of the pool more explicit. I passed pointers to an array to the `getData()` function multiple times and wondered why no data was written to the array. A `getDataCopy()` function would be nice to get a copy of the data in the local pool (with a lock protection for the pool manager override), a `retrieve()` function would also be nice to get a copy and then delete the data (again with lock protection for the pool manager override). Another way would be to lock the pool in the getData() and modifyData() calls for the pool manager by creating a mutex helper on the heap and return a `std::unique_ptr<MutexHelper> mutexLock` to that mutex helper object to the caller. then the caller has access to the locked datapool and is responsible for unlocking the datapool, or unlocking happens automatically when the pointer gets out of scope. Move semantics allow the transfer the ownership of the unique_ptr to the caller. <br> A more simple option would be to forbid `modifyData()` in the StorageManager. UPDATE: I declared the overriden `modifyData()` private and code is still compiling without issues. On another note (maybe sth for another issue): <br> Usually, a message is sent with a storeId to the data in the pool, which is then expected to be deleted. It's easy to forget a deletion operation. If we send around a `std::shared_ptr` instead which is tied to a StorageHelper on the heap containing the store address which calls `deleteData()` automatically on destruction, the deletion would be performed automatically (RAII!). It's a little bit more overhead for a higher safety which does not depend on a developer forgetting something.

muellerr added the

feature

help wanted

question

labels 2020-05-07 13:28:36 +02:00

muellerr changed title from ~~Thread Safety LocalPool read function~~ to StorageManagerIF thoughts

2020-05-07 14:10:37 +02:00

gaisser commented

2020-05-07 18:21:43 +02:00

The Pool Manager implements thread-safe access to functions like reserveSpace / deleteData, the read functions are not protected yet from what I have seen.

Right now, the read functions take pointers and set them to the respective addresses in the local pool.
I've wondered what happens if the read function is called and then the data is changed by another thread/task before doing something with that data. I guess the read functions are not really thread-safe but maybe I have forgotten some details..

You're right the getData call is not thread safe. This was avoided by policy. The used pattern was:

A thread writes to a pool and sends the data away. The reservation of the space must be thread safe because multiple threads may access the pool.
A second thread receives the store address and can get the data. It is now the only owner of the data and must deleted the data if it is no longer in use. It can modify the data with the modifyData call and send it to another one and we are starting with 2. again. The deletion must also be thread safe because it returns the space to the shared pool.

Anyway, a thread-safe implementation of a read call could be implemented like the other PoolManager functions if a copy is performed. I think this would be a nice feature anyway and it would make usage of the pool more explicit.....a retrieve() function would also be nice to get a copy and then delete the data (again with lock protection for the pool manager override).

The data in the pool can be quite large. Usually, we avoid copying such vast amount of data. There are parts of the software, where we actually can't avoid that. The weak enforcment of the deletion call is a problem, so the retrieve call might be interessting for small amounts of data but you have to know the size of the data which is coming in to prelocate an array. But otherwise you should use the modifyData call to modify the data inplace if you want to do modify it.

I passed pointers to an array to the getData() function multiple times and wondered why no data was written to the array. A getDataCopy() function would be nice to get a copy of the data in the local pool (with a lock protection for the pool manager override),

So you passed a pointer instead of a pointer pointer?

Another way would be to lock the pool in the getData() and modifyData() calls for the pool manager by creating a mutex helper on the heap and return a std::unique_ptr<MutexHelper> mutexLock to that mutex helper object to the caller. then the caller has access to the locked datapool and is responsible for unlocking the datapool, or unlocking happens automatically when the pointer gets out of scope. Move semantics allow the transfer the ownership of the unique_ptr to the caller.

A more simple option would be to forbid modifyData() in the StorageManager.
UPDATE: I declared the overriden modifyData() private and code is still compiling without issues.

This will make the pattern of sending data to a third thread or modifying something in place useless. I don't think we make modifyData private. The unique_ptr solution sounds interessting. We should look into that one.

On another note (maybe sth for another issue):

Usually, a message is sent with a storeId to the data in the pool, which is then expected to be deleted. It's easy to forget a deletion operation. If we send around a std::shared_ptr instead which is tied to a StorageHelper on the heap containing the store address which calls deleteData() automatically on destruction, the deletion would be performed automatically (RAII!). It's a little bit more overhead for a higher safety which does not depend on a developer forgetting something.

I don't think that this works. The data of a message is send over a message queue and no C++ Objects survives this very well. We would have to allocate that pointer on the heap and need to delete that one then. So, in this case someone will forget to delete the shared_ptr and thereby forgets to delete the data aswell or maybe I did not get shared_ptr right. We could do something like this: https://stackoverflow.com/questions/25667226/how-can-i-use-shared-ptr-using-postthreadmessage/25671987#25671987

> The Pool Manager implements thread-safe access to functions like reserveSpace / deleteData, the read functions are not protected yet from what I have seen. > > Right now, the read functions take pointers and set them to the respective addresses in the local pool. > I've wondered what happens if the read function is called and then the data is changed by another thread/task before doing something with that data. I guess the read functions are not really thread-safe but maybe I have forgotten some details.. You're right the getData call is not thread safe. This was avoided by policy. The used pattern was: 1. A thread writes to a pool and sends the data away. The reservation of the space must be thread safe because multiple threads may access the pool. 2. A second thread receives the store address and can get the data. It is now the only owner of the data and must deleted the data if it is no longer in use. It can modify the data with the modifyData call and send it to another one and we are starting with 2. again. The deletion must also be thread safe because it returns the space to the shared pool. > Anyway, a thread-safe implementation of a read call could be implemented like the other PoolManager functions if a copy is performed. I think this would be a nice feature anyway and it would make usage of the pool more explicit.....a `retrieve()` function would also be nice to get a copy and then delete the data (again with lock protection for the pool manager override). The data in the pool can be quite large. Usually, we avoid copying such vast amount of data. There are parts of the software, where we actually can't avoid that. The weak enforcment of the deletion call is a problem, so the retrieve call might be interessting for small amounts of data but you have to know the size of the data which is coming in to prelocate an array. But otherwise you should use the modifyData call to modify the data inplace if you want to do modify it. > I passed pointers to an array to the `getData()` function multiple times and wondered why no data was written to the array. A `getDataCopy()` function would be nice to get a copy of the data in the local pool (with a lock protection for the pool manager override), So you passed a pointer instead of a pointer pointer? > Another way would be to lock the pool in the getData() and modifyData() calls for the pool manager by creating a mutex helper on the heap and return a `std::unique_ptr<MutexHelper> mutexLock` to that mutex helper object to the caller. then the caller has access to the locked datapool and is responsible for unlocking the datapool, or unlocking happens automatically when the pointer gets out of scope. Move semantics allow the transfer the ownership of the unique_ptr to the caller. <br> > A more simple option would be to forbid `modifyData()` in the StorageManager. > UPDATE: I declared the overriden `modifyData()` private and code is still compiling without issues. This will make the pattern of sending data to a third thread or modifying something in place useless. I don't think we make modifyData private. The unique_ptr solution sounds interessting. We should look into that one. > On another note (maybe sth for another issue): <br> > Usually, a message is sent with a storeId to the data in the pool, which is then expected to be deleted. It's easy to forget a deletion operation. If we send around a `std::shared_ptr` instead which is tied to a StorageHelper on the heap containing the store address which calls `deleteData()` automatically on destruction, the deletion would be performed automatically (RAII!). It's a little bit more overhead for a higher safety which does not depend on a developer forgetting something. I don't think that this works. The data of a message is send over a message queue and no C++ Objects survives this very well. We would have to allocate that pointer on the heap and need to delete that one then. So, in this case someone will forget to delete the shared_ptr and thereby forgets to delete the data aswell or maybe I did not get shared_ptr right. We could do something like this: https://stackoverflow.com/questions/25667226/how-can-i-use-shared-ptr-using-postthreadmessage/25671987#25671987

muellerr commented

2020-05-07 18:41:00 +02:00

The policy makes perfect sense. It would be good to document it in the pool manager. Interesting note: making modifyData() private compiled, but the unit test failed.

I actually passed pointer to pointer, but the underlying pointer was retrieved with the data() function of a std::array.
SOmething along the lines of

// works not
uint8_t * pointer = receptionArray.data();
result = SimplePool.modifyData(testStoreId, &pointer, &size);
REQUIRE(result == retval::CATCH_OK);
for(size_t i = 0; i < size; i++) {
	// fails here
	CHECK(receptionArray[i] == i );
}

// works
result = SimplePool.modifyData(testStoreId, &pointer, &size);
memcpy(receptionArray.data(), pointer, size);
REQUIRE(result == retval::CATCH_OK);
for(size_t i = 0; i < size; i++) {
	CHECK(receptionArray[i] == i );
}

(see Unit Tests)
I will have a look at it again...
After some research, I have found out that std::array is not assignable.

I can look into the unique_ptr solution.Question is: what happens if the caller tries to call getData() twice and in the same scope and forgets to reset the first unique_ptr... there propably will be a mutex lock error, right?

I tried some experiments:
http://cpp.sh/9ihry
Just like you said, it's allocated on the heap. A not very elegant combination of new, move semantics, and other stuff, but it deletes itself after going out of scope. However, sending a pointer via a message queue is kind of annoying. Pointers can have different sizes.. I will test this with freeRTOS (32 bit) and on linux(64 bit), but I think I had some issues with creating mqueues on linux..

The policy makes perfect sense. It would be good to document it in the pool manager. Interesting note: making modifyData() private compiled, but the unit test failed. I actually passed pointer to pointer, but the underlying pointer was retrieved with the data() function of a std::array. SOmething along the lines of ```cpp // works not uint8_t * pointer = receptionArray.data(); result = SimplePool.modifyData(testStoreId, &pointer, &size); REQUIRE(result == retval::CATCH_OK); for(size_t i = 0; i < size; i++) { // fails here CHECK(receptionArray[i] == i ); } // works result = SimplePool.modifyData(testStoreId, &pointer, &size); memcpy(receptionArray.data(), pointer, size); REQUIRE(result == retval::CATCH_OK); for(size_t i = 0; i < size; i++) { CHECK(receptionArray[i] == i ); } ``` (see Unit Tests) I will have a look at it again... After some research, I have found out that std::array is not assignable. I can look into the unique_ptr solution.Question is: what happens if the caller tries to call getData() twice and in the same scope and forgets to reset the first unique_ptr... there propably will be a mutex lock error, right? I tried some experiments: http://cpp.sh/9ihry Just like you said, it's allocated on the heap. A not very elegant combination of new, move semantics, and other stuff, but it deletes itself after going out of scope. However, sending a pointer via a message queue is kind of annoying. Pointers can have different sizes.. I will test this with freeRTOS (32 bit) and on linux(64 bit), but I think I had some issues with creating mqueues on linux..

muellerr referenced this issue

2020-05-11 12:06:16 +02:00

StorageManager thread-safe modifyData implementation #60

muellerr referenced this issue

2020-05-11 12:09:44 +02:00

Thread-Safe and RAII conformant getData() and modifyData() pool calls #74

muellerr commented

2020-07-06 12:51:10 +02:00

Okay, this has been basically reduced to the new Accessor objects.

muellerr referenced a pull request that will close this issue

2020-07-06 12:51:55 +02:00

New StoreAccessors and separation of implementation for storagemanager classes #92