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 behaves strange #776

Closed
Extrawurst opened this Issue Aug 10, 2014 · 12 comments

Comments

Projects
None yet
4 participants
@Extrawurst
Contributor

Extrawurst commented Aug 10, 2014

ok i am trying to figure out how to use the pubsub implementation for redis. seems the redis test contains the only currently working example.. BUT as soon as i add a sleep(1.seconds); at:
https://github.com/rejectedsoftware/vibe.d/blob/master/tests/redis/source/app.d#L80

right after starting to listen. the whole test never finishes and hangs...

how is one supposed to use the listen method to never stop listening to messages for the whole program runtime ?

@s-ludwig

This comment has been minimized.

Show comment
Hide comment
@s-ludwig

s-ludwig Aug 13, 2014

Member

I'm currently under time pressure because of two external projects, so it will take some days for me to have a closer look. But it looks like the current API is at least a little weird regarding when and where to call listen and subscribe. But maybe @etcimon or @metafex have an idea?

Member

s-ludwig commented Aug 13, 2014

I'm currently under time pressure because of two external projects, so it will take some days for me to have a closer look. But it looks like the current API is at least a little weird regarding when and where to call listen and subscribe. But maybe @etcimon or @metafex have an idea?

@metafex

This comment has been minimized.

Show comment
Hide comment
@metafex

metafex Aug 13, 2014

Contributor

I'm looking at it right now, tried the examples but it doesn't seem to work anymore. Now who didn't test their changes to the source ;-) If no one is opposed to it I'll try to patch it up and submit a PR.

Contributor

metafex commented Aug 13, 2014

I'm looking at it right now, tried the examples but it doesn't seem to work anymore. Now who didn't test their changes to the source ;-) If no one is opposed to it I'll try to patch it up and submit a PR.

@s-ludwig

This comment has been minimized.

Show comment
Hide comment
@s-ludwig

s-ludwig Aug 13, 2014

Member

If I remember right, I've added the assertions - this seems to be what causes the example to fail. They are necessary though, because before starting to listen, the connection isn't locked, yet, so that the subscribe commands could go to a different connection that the one that later listens.

Member

s-ludwig commented Aug 13, 2014

If I remember right, I've added the assertions - this seems to be what causes the example to fail. They are necessary though, because before starting to listen, the connection isn't locked, yet, so that the subscribe commands could go to a different connection that the one that later listens.

@s-ludwig

This comment has been minimized.

Show comment
Hide comment
@s-ludwig

s-ludwig Aug 13, 2014

Member

BTW, I'd like to slightly change the API, so that it's impossible to use it in a wrong way. For example like this:

RedisDatabase db;
auto subscriber = db.listen({ ... }); // starts a background task and locks the connection
subscriber.subscribe(...); // make subscriptions

RedisSubscriber.this() would then be made private.

Member

s-ludwig commented Aug 13, 2014

BTW, I'd like to slightly change the API, so that it's impossible to use it in a wrong way. For example like this:

RedisDatabase db;
auto subscriber = db.listen({ ... }); // starts a background task and locks the connection
subscriber.subscribe(...); // make subscriptions

RedisSubscriber.this() would then be made private.

@Extrawurst

This comment has been minimized.

Show comment
Hide comment
@Extrawurst

Extrawurst Aug 13, 2014

Contributor

@s-ludwig I very much like this kind of API !

Contributor

Extrawurst commented Aug 13, 2014

@s-ludwig I very much like this kind of API !

etcimon added a commit to etcimon/vibe.d that referenced this issue Aug 13, 2014

@etcimon

This comment has been minimized.

Show comment
Hide comment
@etcimon

etcimon Aug 13, 2014

Contributor

Sorry for that, I'm to blame on this one ;)

Contributor

etcimon commented Aug 13, 2014

Sorry for that, I'm to blame on this one ;)

etcimon added a commit to etcimon/vibe.d that referenced this issue Aug 13, 2014

@metafex

This comment has been minimized.

Show comment
Hide comment
@metafex

metafex Aug 13, 2014

Contributor

Ok, I have been debugging this whole thing for two hours straight now and have come to a conclusion: The multi-subscribe with the "impossible to use it in a wrong way" style API is one son of a bitch ;-) My problem is, that I don't know from redis how many parts of a reply are being returned. Anybody got an idea how to do this in a nice way?
The only way I see to solve this, is to either lock blisten and read the reply in subscribe or to tell it somehow that a subscribe-response is waiting with n elements.

Contributor

metafex commented Aug 13, 2014

Ok, I have been debugging this whole thing for two hours straight now and have come to a conclusion: The multi-subscribe with the "impossible to use it in a wrong way" style API is one son of a bitch ;-) My problem is, that I don't know from redis how many parts of a reply are being returned. Anybody got an idea how to do this in a nice way?
The only way I see to solve this, is to either lock blisten and read the reply in subscribe or to tell it somehow that a subscribe-response is waiting with n elements.

@etcimon

This comment has been minimized.

Show comment
Hide comment
@etcimon

etcimon Aug 13, 2014

Contributor

I don't know from redis how many parts of a reply are being returned. Anybody got an idea how to do this in a nice way?

Would be easy to debug with an example, I haven't tested this extensively yet as I've been busy rewriting external libraries in native D to get single-binary builds working. Care to share the code for this bug?

Contributor

etcimon commented Aug 13, 2014

I don't know from redis how many parts of a reply are being returned. Anybody got an idea how to do this in a nice way?

Would be easy to debug with an example, I haven't tested this extensively yet as I've been busy rewriting external libraries in native D to get single-binary builds working. Care to share the code for this bug?

@metafex

This comment has been minimized.

Show comment
Hide comment
@metafex

metafex Aug 13, 2014

Contributor

@etcimon Nothing much and with some debug statements: https://gist.github.com/metafex/5023b60824d8850d7dd3 (diff based on master, I'm too lazy to create a branch ^^)

Contributor

metafex commented Aug 13, 2014

@etcimon Nothing much and with some debug statements: https://gist.github.com/metafex/5023b60824d8850d7dd3 (diff based on master, I'm too lazy to create a branch ^^)

@etcimon

This comment has been minimized.

Show comment
Hide comment
@etcimon

etcimon Aug 13, 2014

Contributor

Looks good, you should probably add my changes though because currently, a duration 0 causes the listener to abort immediately

I don't know from redis how many parts of a reply are being returned. Anybody got an idea how to do this in a nice way?

Currently, the listener task should take care of calling the delegate with each part. It's a bit like a listenHTTP. The only real way to dispatch those is to put a router on top of it...

Contributor

etcimon commented Aug 13, 2014

Looks good, you should probably add my changes though because currently, a duration 0 causes the listener to abort immediately

I don't know from redis how many parts of a reply are being returned. Anybody got an idea how to do this in a nice way?

Currently, the listener task should take care of calling the delegate with each part. It's a bit like a listenHTTP. The only real way to dispatch those is to put a router on top of it...

@metafex

This comment has been minimized.

Show comment
Hide comment
@metafex

metafex Aug 13, 2014

Contributor

Ok cool, I'll base of your changes and will try to come up with a nice and clean solution for this.

Contributor

metafex commented Aug 13, 2014

Ok cool, I'll base of your changes and will try to come up with a nice and clean solution for this.

@etcimon

This comment has been minimized.

Show comment
Hide comment
@etcimon

etcimon Aug 13, 2014

Contributor

will try to come up with a nice and clean solution for this.

Extending the comparison from listenHTTP, maybe some other objects could also be returned in the call (RedisSubscriber, RedisClient, etc). Also, trying to use yield/resume on an existing task waiting for an event from published data is a bit tricky, you need to build a protocol on top of the pubsub model (serialization library & msg structure) to identify which task to wake up. Maybe there could be some idioms there.

Contributor

etcimon commented Aug 13, 2014

will try to come up with a nice and clean solution for this.

Extending the comparison from listenHTTP, maybe some other objects could also be returned in the call (RedisSubscriber, RedisClient, etc). Also, trying to use yield/resume on an existing task waiting for an event from published data is a bit tricky, you need to build a protocol on top of the pubsub model (serialization library & msg structure) to identify which task to wake up. Maybe there could be some idioms there.

@s-ludwig s-ludwig closed this in eeab6ce Aug 13, 2014

s-ludwig added a commit that referenced this issue Aug 13, 2014

Merge pull request #781 from etcimon/redis-pubsub-fix
Add non-zero timeout verification. Fixes #776.

Also removes the RedisDebug version from the Redis integration test to sidestep Travis CI woes.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment