Several improvements to PubSub.parse_response() method.

[gward]

In trying to understand blocking/timeout semantics, I think I found an easier way to do things, and I found a bug.

[gward]

Ping...? @andymccurdy, I'd love some feedback on this. I think it's a nice little cleanup, but I can see how a reasonable person might object to certain aspects of the PR. Would like to hear what you think!

(More importantly, I also want to know if I can rely on get_message(timeout=None) for to block indefinitely, which is an undocumented feature of redis-py now. I'd like to make it an official documented feature.)

[pfreixes]

Hi @gward the PR seems rationale, with your changes the parameters regarding the timeout are aligned with well reputed specifications [1] and clearly most known used.

IMHO I would keep the blog=True [2] to keep backward compatibility due that it is a public method and somebody might trust in the default keyword values. To make it possible we should change also the parameters values used by listen and get_message with block=None and then use the proper value for the timeout regarding each call. fair enough?

About the tests, for me they are a bit fragile and hard to keep them deterministic - tried to contain with a 0.005 fuzz value. Redis-py implements integration tests instead of unit tests, therefore the communications with the Redis server are almost a must, hereby if a test get nailed because timeout is not well implemented it will mean that this test is broken.

I would suggest rewrite these tests taking into account this situation, getting just those tests that are deterministic and maintainable, perhaps test_async_response_message and test_timeout_response_messages to assert that the time passed between the call to response_message is less than 1 second in the first test and bigger for the second one using 1 second as a timeout value.

How does it look for you?

[1] http://linux.die.net/man/2/select
[2] https://github.com/andymccurdy/redis-py/pull/707/files#diff-085cbe85ef9bc2ad832d79163eb39172L2178

[gward]

@pfreixes thank you for the feedback! I'm leaving for 10 days of vacation soon, so won't be able to handle everything right now. But your changes to parse_response() etc. sound good to me.

I need to spend some time thinking about the tests. I really want to retain the test that reproduces the bug I found. Apart from that, I'm open to suggestions. I have no idea how to test network-timing-dependent code without fuzzy comparisons and without some degree of non-determinism. If you can point out existing tests in redis-py that do this, I will read them carefully to learn how it's done.

[pfreixes]

Hi @gward, regarding the bug and its test that its related with #716 I would suggest send a MR detached from the current one to make it mergeable easily and properly related with the issue already opened, @andymccurdy will appreciate it.

About the tests, remember that integration tests are not unit test and the test pyramid principle [1] says that the cases covered by integration or acceptance test are just those ones related with the user feature. IMHO I will keep them as much simple as it possible. Im still convinced that we should cover the test_async_response_message and test_timeout_response_messages without cover them with defensive code - sigalarm, fuzz, ...

[1] http://martinfowler.com/bliki/TestPyramid.html

[carltongibson]

• I'm not sure if the API change here is worth it.
  • The blocks parameter is kind of nice. It's clear. I don't have to remember what 0 and None mean. Do I pass -1 to block?. Equally, There should be correct one way to do things... — so just having timeout is a win from that perspective.
    • But who's calling parse_response directly here? No-one probably, so it's how the first level API is affected: listen and get_message remain unchanged.
    • The docs strings there probably need adjustment.
• As it stands the default behaviour of parse_response is reversed, from blocking to non-blocking. (If the timeout default were None this would be avoided.

@gward I didn't understand the bug you refer to just be looking at it. Could you explain? (Sorry if I'm being slow.)

[github-actions]

This pull request is marked stale. It will be closed in 30 days if it is not updated.