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.

Sign up for GitHub

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

Jump to bottom

Added timeout non-zero verification - fixes #776 #781

Merged
merged 1 commit into from
Aug 13, 2014
Merged

Added timeout non-zero verification - fixes #776 #781

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 12 additions & 6 deletions source/vibe/db/redis/redis.d
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -488,14 +488,18 @@ final class RedisSubscriber {

bool bstop(){
if (!stop()) return false;
while (m_listening) sleep(1.msecs);
while (m_listening) {
sleep(1.msecs);
}
Copy link
Member

Choose a reason for hiding this comment

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

I guess this is left over from debugging?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, the sleep timer creates a new one every msec, it reaches 4500 IDs by the time the server is stopped

Copy link
Member

Choose a reason for hiding this comment

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

Do you think that this is performance relevant? Timer creation is usually nothing more than assigning a few bytes of memory and incrementing a counter (plus some modulus calculation in the hash map).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's not very relevant for performance, I was trying to figure out where all those timers came from when tracing and afterwards, I realized it would have been easier to debug it if it showed up as the same ID.

Copy link
Member

Choose a reason for hiding this comment

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

So it is in fact a leftover from debugging ;)
I think we should solve this at a lower level. This just makes the code less readable for no obvious gain (apart from making timer debugging slightly easier when using this specific Redis functionality, which should be a highly uncommon case). There could for example be a special per-task default timer that is used by sleep.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm, I had a feeling the proposed solution would be at a lower level.

return true;
}

bool stop(){
if (!m_listening)
return false;
m_stop = true;
// todo: publish some no-op data to wake up the listener?

return true;
}

Expand Down Expand Up @@ -567,7 +571,7 @@ final class RedisSubscriber {
sw.start();
while (!gotData){
if (m_lockedConnection.conn.waitForData(5.seconds)) gotData = true;
if (sw.peek().seconds > timeout.total!"seconds") { gotData = false; break; }
if (timeout > 0.seconds && sw.peek().seconds > timeout.total!"seconds") { gotData = false; break; }
if (m_stop){ gotData = false; break; }
}
sw.stop();
Expand All @@ -592,10 +596,10 @@ final class RedisSubscriber {

auto ln = cast(string)m_lockedConnection.conn.readLine();
string cmd;
if (ln[0] == "$"[0]){
if (ln[0] == '$'){
cmd = cast(string)m_lockedConnection.conn.readLine();
}
else if (ln[0] == "*"[0]) {
else if (ln[0] == '*') {
cmd = getString();
}else {
enforceEx!RedisProtocolException(false, "expected $ or *");
Expand Down Expand Up @@ -676,8 +680,10 @@ final class RedisSubscriber {
auto task = runTask({
blisten(callback, timeout);
});
import std.datetime;
while(!m_listening) sleep(1.usecs);
import std.datetime : usecs;
while (!m_listening) {
sleep(1.usecs);
}
return task;
}
}
Expand Down
2 changes: 1 addition & 1 deletion tests/redis/dub.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,5 +4,5 @@
"dependencies": {
"vibe-d": {"version": "~master", "path": "../../"}
},
"versions": ["VibeCustomMain", "RedisDebug"]
"versions": ["VibeCustomMain"]
}
4 changes: 2 additions & 2 deletions tests/redis/source/app.d
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -67,8 +67,8 @@ void runTest()
logInfo("LISTEN Recv Channel: %s, Message: %s", channel.to!string, msg.to!string);
logInfo("LISTEN Recv Time: %s", Clock.currTime().toString());
});

assert(sub.isListening);
sleep(1.seconds);
sub.subscribe("SomeChannel");

logInfo("PUBLISH Sent: %s", Clock.currTime().toString());
Expand All @@ -84,7 +84,7 @@ void runTest()

int main()
{
int ret = 0;
int ret = 0;
runTask({
try runTest();
catch (Throwable th) {
Expand Down