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

Redis pubsub fixes #2407

Merged
merged 4 commits into from Feb 11, 2020
Merged

Redis pubsub fixes #2407

merged 4 commits into from Feb 11, 2020

Conversation

yazd
Copy link
Contributor

@yazd yazd commented Jan 27, 2020

This fixes two issues in redis pubsub implementation.

  • The first issue (solved in 1st commit) is an infinite loop (that causes 100% CPU) that happens when the redis server exits while listening to pubsub messages.
    The following code shows a reproduction case:
import vibe.db.redis.redis;
import vibe.core.core;
import vibe.core.log;

void main() {
	runTask(&listenRedis);
	runApplication();
}

void listenRedis() {
	auto redis = connectRedis("127.0.0.1");
	auto subs = redis.createSubscriber();
	subs.subscribe("test");

	logInfo("subscribed");

	auto task = subs.listen((string channel, string message) {
		logInfo("received message on %s: %s", channel, message);
	});
	task.join();

	logInfo("done listening");
}

Redis server must be stopped to face the issue.

  • Second issue (solved in 2nd commit) is that the task doesn't join back or terminate because the code in teardown was waiting for Action.STOP message, but that will never arrive, because it was already consumed by the handler. I don't think there are cases when that is not the case.

Copy link
Member

@s-ludwig s-ludwig left a comment

Although it is a bit difficult to keep track of all involved code paths, the change looks good to me! I'd just modify the test case for added robustness.

tests/vibe.db.redis.redis.pr2407/source/app.d Outdated Show resolved Hide resolved
@dlang-bot dlang-bot merged commit 4a894e1 into vibe-d:master Feb 11, 2020
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants