Fixes #30
Fixes #30
Conversation
src/RWLock.h
Outdated
|
|
||
| uint16_t xadd(volatile uint16_t &m, uint16_t v) | ||
| { return ::xadd<uint16_t>(m, v); } | ||
| void atomic_and(volatile uint16_t &m, uint16_t v) | ||
| { ::atomic_and<uint16_t>(m, v); } | ||
|
|
||
| volatile uint16_t _rw_lock; | ||
| unsigned _max_attempts; |
There was a problem hiding this comment.
This should be const (since it is initialized in the constructor). This may allow compiler to improve code generation in the methods where it is used.
src/RWLock.h
Outdated
|
|
||
| uint16_t xadd(volatile uint16_t &m, uint16_t v) | ||
| { return ::xadd<uint16_t>(m, v); } | ||
| void atomic_and(volatile uint16_t &m, uint16_t v) | ||
| { ::atomic_and<uint16_t>(m, v); } | ||
|
|
||
| volatile uint16_t _rw_lock; | ||
| unsigned _max_attempts; |
There was a problem hiding this comment.
I don't see where _max_attempts is used.
There was a problem hiding this comment.
I guess it's not clear that this is the comment that caused me to withhold approval. The other comments are questions/suggestions.
| @@ -119,6 +119,7 @@ namespace VDMS { | |||
|
|
|||
| public: | |||
| static void init(); | |||
| static void destroy(); | |||
There was a problem hiding this comment.
just pairs up with an init
There was a problem hiding this comment.
Okay, I can see that. But init is needed (I assume) because the information required to perform initialization isn't available at construction time. The same concern doesn't apply to using a destructor, and I think performing this cleanup work in the destructor would be preferable.
There was a problem hiding this comment.
As you pointed out, the object is a static member, so the destructor isn't called at the time when this destroy function is called, so this cannot be changed to a destructor.
src/PMGDQueryHandler.cc
Outdated
| @@ -57,6 +57,16 @@ void PMGDQueryHandler::init() | |||
| _dblock = new RWLock(); | |||
| } | |||
|
|
|||
| void PMGDQueryHandler::destroy() | |||
| { | |||
| if (_db) { | |||
Edit multiple add test in python to add slowdown between threads to avoid connection refusal due to queueing.
| @@ -80,7 +80,9 @@ namespace VDMS { | |||
| } | |||
|
|
|||
| public: | |||
| RWLock() : _rw_lock(0) {} | |||
| static const unsigned MAX_ATTEMPTS = 10; | |||
There was a problem hiding this comment.
This should be called DEFAULT_MAX_ATTEMPTS.
| simultaneous = 1000; | ||
| thread_arr = [] | ||
| for i in range(1,simultaneous): | ||
| thread_add = Thread(target=self.addEntity,args=(i,) ) | ||
| thread_add.start() | ||
| thread_arr.append(thread_add) | ||
| time.sleep(0.002) |
There was a problem hiding this comment.
I am not convinced we should change the test so that it passes.
The introduction of the RWlock and msync should not break this tests, we should push a fix to address this, not change the tests so that we mask it.
Should we create an separate issue for this? Or want to address it here?
There was a problem hiding this comment.
I think we can create a separate issue. What I noticed here is....msync increases how long PMGD takes. That means the reader writer lock has more timeouts and so more threads wait. That somehow seems to be messing with some queue that starts denying newer threads from even getting connected. Needs more exploring which I don't want to hold up this pull request for.
There was a problem hiding this comment.
Also, one of the things we need to figure now at the VDMS layer is that timeout based locking is bound to cause more failures since threads don't get to wait however long. Do we introduce internal retries or let the client retry? We should of course try to figure out the right values as we run more experiments but just saying.
| @@ -80,7 +80,9 @@ namespace VDMS { | |||
| } | |||
|
|
|||
| public: | |||
| RWLock() : _rw_lock(0) {} | |||
| static const unsigned MAX_ATTEMPTS = 10; | |||
In case of errors, the lack of PMGDQueryHandler pointer cleanup was causing problems. And not being able to set the number of attempts for RWLock was annoying for some application use cases. This pull request address both the problems.