PubSub Implementation about miniredis HOT 19 CLOSED

Hi! Sorry, this PR dropped to the bottom of my todolist somehow. I'll try to have a good look soon, it's a long pr :)

from miniredis.

I have not tried this, so I have no advise, so go for it.
Do have a look at the tests in ./integration/, and please add as many tests as needed there.

Have fun! Let me know if I can assist in any way. I'm happy to look at work-in-progress PRs if you need feedback.

from miniredis.

See #49.

from miniredis.

@Al2Klimov it seems that your fork is unmergeable to upstream and cannot be built

# (I got the following error)
# github.com/Al2Klimov/miniredis
../../Al2Klimov/miniredis/direct.go:967:6: error: reference to undefined field or method ‘MsgQueue’
  peer.MsgQueue.Enqueue(&queuedPubSubMessage{channel, message})
      ^
../../Al2Klimov/miniredis/miniredis.go:169:3: error: reference to undefined field or method ‘OnDisconnect’
  s.OnDisconnect(m.onDisconnect)

Do you use your fork?

from miniredis.

Yes, the upstream master.

from miniredis.

Not really, I'm not sure it fits the 'unit test' use case this thing is designed for. That said, if you need it I don't mind having a look to see if it's easy to add.

from miniredis.

In my case, I have a client that consumes the Redis pubusb functionality which I wanted to unit test. I spent the greater part of yesterday working on an implementation of it in my fork but that opened up a whole can of worms. If you'd like to dig deeper into it, I wouldn't mind sharing what I have and collaborating on it.

For my immediate use case, I decided to use redeo to capture the pubsub requests and make sure they were executed properly

from miniredis.

I had a better look. Implementing the commands themselves in miniredis is no problem, but for a 'SUBSCRIBE' command we need to send an arbitrary number of messages to the client, but those messages are not a direct response to a command. Unfortunately redeo doesn't support sending messages which are not a response to a command.

If you have a look at https://github.com/bsm/redeo/blob/master/server.go#L141 you'll see that it does an Apply(), which handles the command->reply logic, the result of that is written to the socket after the whole reply is generated. This means that in the methods in miniredis which implement the commands you can't keep the connection 'open'. You have to return something, and the reply will only be send after the callback is done. Messages are always command->reply. I also see no obvious way to add a 'Flush()' method to redeo, which would make this possible.

I think your approach of using redeo to at least have a look at the commands makes sense, and from there on do proper integration tests :(

from miniredis.

Yeah, I noticed at the end of all my ventures yesterday that the subscribe call would require either another server that can keep connections open or a rework of the internal server that miniredis is using.

I came to the same conclusion as well. In any case though, awesome job on this project! It's well designed and saved me so much time! 👍 :)

from miniredis.

Hello @alicebob and thank you for looking at this.

We – @Icinga – are developing a program that deals with a lot of Redis events and we'd like to get a unit test coverage near 100%.

Do I understand you right that you can't implement this due to a problem with a dependency/framework/vendorlib of miniredis?

Best,
AK

from miniredis.

Hello Alexander,

this was true back in the days, but I've switched the server part to a custom implementation. I think it should be possible to implement now.
That said, I doubt I have time for this in the very near future.

from miniredis.

Hello @alicebob and thank you for your quick reply!

Just got the permission to implement this myself. Should I take care about anything special like "don't do it like x, y or z as this have already been a problem in the past" or do you just give me green light for this?

Best,
AK

from miniredis.

I'm going to implement the commands {,P}{,UN}SUBSCRIBE and PUBLISH. Shall all of them be also executable from the backend like m.Publish(c, m)?

from miniredis.

It sounds great if they are also directly executable, especially if they are published as Go channels.

I can image that it would come in handy when you make something which publishes a stream of notifications on redis, and you can read from a Go channel in the test to check what's going on. But, if you don't have any use-case for this yourself we can also add it later.

from miniredis.

Hey guys @Al2Klimov @alicebob!
how is the implementation of PUBLISH going? It would help me a lot.
Can I anyhow help to get it merged?

from miniredis.

1. Both works for me.
2. We only use features merged into the master.

from miniredis.

Thanks for the hint! I used the feature branch.

from miniredis.

What do you mean by "master"? Al2Klimov/miniredis' master doesn't seem to contain any recent commits by you, all the pubsub stuff seems to be in feature/pubsub-1 branch. Do you mean you use the upstream's master?

from miniredis.

Regarding my comment, my mistake was that I didn't rename Al2Klimow/miniredis to alicebob/redis and go uses absolute paths, so modified files like server.go were not used, go used the original alicebob's files instead.

@Al2Klimov it seems that your fork is unmergeable to upstream and cannot be built

# (I got the following error)
# github.com/Al2Klimov/miniredis
../../Al2Klimov/miniredis/direct.go:967:6: error: reference to undefined field or method ‘MsgQueue’
  peer.MsgQueue.Enqueue(&queuedPubSubMessage{channel, message})
      ^
../../Al2Klimov/miniredis/miniredis.go:169:3: error: reference to undefined field or method ‘OnDisconnect’
  s.OnDisconnect(m.onDisconnect)

Do you use your fork?

from miniredis.