-
Notifications
You must be signed in to change notification settings - Fork 301
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
HPCC-32781 RoxieSocketQueueManager::run may be blocked by actCrit #19171
base: master
Are you sure you want to change the base?
HPCC-32781 RoxieSocketQueueManager::run may be blocked by actCrit #19171
Conversation
b765bca
to
cd7ce5b
Compare
716badc
to
e55df5b
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@richardkchapman two minor comments.
I was debating which version to target, I think 9.10 is probably correct. The only item that might be moving to 9.6.x would be changes removing the release of packet outside of the critical block.
} | ||
|
||
void abortChannel(unsigned channel) | ||
inline void setPacket(IRoxieQueryPacket *p) | ||
{ | ||
CriticalBlock b(actCrit); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should probably use the same pattern as set Activity to avoid releasing in the crit sec:
Owned<X> temp(p);
{
CriticalSection x;
packet.swap(temp);
and also worth searching the entire source code for similar examples.
roxie/ccd/ccdqueue.cpp
Outdated
@@ -1316,6 +1317,7 @@ class RoxieQueue : public CInterface, implements IThreadFactory | |||
void wait() | |||
{ | |||
idle++; | |||
CLeavableCriticalBlock b(availCrit, limitWaitingWorkers); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
trivial: The (atomic) decrement of idle could be outside the critical block
e55df5b
to
e2c6a68
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please associated the PR with a jira, update the commit messages and change to non-draft
e2c6a68
to
0e9c67a
Compare
Avoid need to obtain critsec just to check if a worker thread's packet matches Signed-off-by: Richard Chapman <rchapman@hpccsystems.com>
Signed-off-by: Richard Chapman <rchapman@hpccsystems.com>
This was not used (I hope!) and was desiged to solve an issue that was better managed by the IBYTI delay logic. Signed-off-by: Richard Chapman <rchapman@hpccsystems.com>
0e9c67a
to
32402f5
Compare
Type of change:
Checklist:
Smoketest:
Testing: