Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

Add read/write timeout support #33

Closed
wants to merge 4 commits into
from

Conversation

Projects
None yet
6 participants

Ovid commented Jan 22, 2013

Hi Pedro,

Due to our needs at $work, this pull request adds timeout support to read/write operations. However, I am not entirely comfortable with my code or the tests, so any comments/review you have would be great.

Cheers,
Ovid

You trigger reconnect with a write timeout. Is this what you really want? I mean, if you don't want to wait more than X miliseconds, reconnecting is probably not what you have in mind.

Owner

Ovid replied Jan 22, 2013

melo, I'm a bit confused. The _throw_reconnect doesn't reconnect, does it? It only appears to die.

Oh, wait. I see that you catch the exception and reconnect. Duh. The name should have made it clear to me. Yes, this should probably confess here.

melo replied Jan 22, 2013

Exactly.

I'm guessing that auto-reconnect and read/write timeout combination should have an extra note saying that:

  • we'll reconnect if we detect a EOF before reading/writing: the code is already there;
  • we'll throw an exception if we timeout during read/write.

Right now, it seems the most logical combination. If you do care a lot about delay inside Redis.pm, disable reconnect, and make sure the connection is alive once at the beginning of your request (for example); or use a very small reconnect (40ms, with 10ms between tries for ex.)...

Either way, your choice.

Owner

melo commented Jan 22, 2013

I like the idea. I'll probably merge it but I'm thinking on doing the following:

  • use the "with timeout" code path always, default timeout is 0, so it blocks;
  • merge SpawnFakeRedis.pm into SpawnTestRedis.pm as fake_redis(): lot's of common code in there.

There is just one point I still don't know what is best: reconnecting and write timeouts... Not sure what to do there...

They could serve different opposing goals.

Ovid commented Jan 22, 2013

If you merge the Spawn* modules, I'd love to see your version of Fake Redis. Mine was a terrible hack to try and get something working without taking forever to implement it.

Owner

melo commented Jan 22, 2013

I'm merging just to reuse reap() and friends, the actual implementation of fake redis you came up with works fine by me :)

I might make some cosmetic tweaks, but nothing more.

halkeye commented Apr 10, 2013

We had this issue at $work today as well. A redis server became unresponsive and our webapp started to time out trying to do a GET. I would really like to see this in the official module. Is there anything I can do to help or is simply a bump needed.

Owner

melo commented Apr 10, 2013

I don't have the time to finishing merging this right now. I'll try and get to it in the weekend.

If someone wants to look at the entire thread on this issue and cleanup the pull request so that it merges cleanly, and has tests, I'll merge it and release that.

Ovid commented Apr 10, 2013

I would love to do that. However, I'm busy starting a freelancing company and writing talks for YAPC::NA. I'm afraid my tuits are running low :/

Member

tsee commented Jun 6, 2013

Bump. Any chance you'll get to polishing this up and including it in a CPAN release?

Owner

melo commented Jun 6, 2013

As the code stands, it breaks the hacks we did to work on windows...

I don't have tuits to fix that at the moment.

estrabd commented Jun 6, 2013

What functionality is this adding, the ability to add a default timeout or handling a situation where the client hangs indefinitely because of a network blip or server outage during a read/write?

Owner

dams commented Oct 1, 2013

I've re-implemented this feature in a much more robust and less intrusive way, and it should work on any platform. See #52

Owner

dams commented Feb 17, 2014

This PR has been better implemented since then. Closing

@dams dams closed this Feb 17, 2014

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment