perl-redis icon indicating copy to clipboard operation
perl-redis copied to clipboard

Join wait_all_responses() and wait_for_messages()

Open melo opened this issue 13 years ago • 2 comments

Right now wait_all_responses() and wait_for_messages() are incompatible, and shouldn't be. We should have wait_for_SOMETHING that does both.

The SOMETHING part is open for discussion. Not sure what to call it yet...

Feedback appreciated.

melo avatar Jun 18 '12 13:06 melo

Hi, sorry it took me a while to look at this — last week got busy.

So, on the naming question: maybe something like wait_for_responses or wait_for_input would work; or maybe even wait_for_pending, finessing the question of what the appropriate noun is. Or s/wait_for/await/ to shave three characters and one word off each name.

As for the behaviour: I assume that in non-subscriber mode we'd also support the $timeout argument with essentially the same meaning (zero means "never cede control to the caller", positive means "cede control after idle for this many seconds"), except that in non-subscriber mode we always cede control anyway once all pending callbacks have been invoked.

It may well be the case that there are performance advantages for the implementation of the unified method to special-case the zero-timeout case to avoid IO::Select — just $self->__read_response forever (or until we run out of queued non-subscriber callbacks). Naïvely, that sounds like a win — for each response, we'd save one select, one read, one (buffered) ungetc, and (depending on platform) either two ioctls or two fcntls. It's probably worth benchmarking it to see whether the saving is big enough to justify the extra implementation complexity.

Does that help?

arc avatar Jun 24 '12 15:06 arc

No worries, appreciate the feedback.

As for naming, its a small issue. I like await_pending actually, but I'll let this sink in a bit more.

Yes, the behavior would be as you describe: $timeout is honored, and in non-subscriber mode, we would exit after all pending responses are received.

The rest of your comments are well taken, and yes it helps me clear somethings.

I'll try and code this up on a separate branch. Long term, I need to extract all the network code into a separate ::PP class, as I would like to provide a hiredis-based XS backend for Redis.pm, so I'll consider moving the network code out now. Maybe this would make Redis.pm itself simpler.

melo avatar Jun 26 '12 10:06 melo