Skip to content
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

add support for timeout in constructor #14

Merged
merged 3 commits into from Oct 6, 2015

Conversation

@russellpierce
Copy link
Contributor

@russellpierce russellpierce commented Oct 6, 2015

If you specify an incorrect address as it stands it seems as though RcppRedis will wait for a very long time (if not indefinitely) before throwing an error. This modification allows one to set a timeout in the constructor. If the connection has not been established by the timeout, it gives up and produces an error.

Russell-DataScience and others added 3 commits Oct 5, 2015
Merge branch 'master' of github.com:drknexus/rcppredis

Conflicts:
	src/Redis.cpp
@eddelbuettel
Copy link
Owner

@eddelbuettel eddelbuettel commented Oct 6, 2015

Sounds reasonable. But (just to think out aloud) should we not just always set it to, say, 500ms (or three seconds or whatnot) ?

Also, maybe supply the argument as a double and split into whole int and millisecond?

@eddelbuettel
Copy link
Owner

@eddelbuettel eddelbuettel commented Oct 6, 2015

I think I just add that. Hold on ...

eddelbuettel added a commit that referenced this pull request Oct 6, 2015
add support for timeout in constructor
@eddelbuettel eddelbuettel merged commit 84d5dcd into eddelbuettel:master Oct 6, 2015
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@russellpierce
Copy link
Contributor Author

@russellpierce russellpierce commented Oct 6, 2015

Do I set a smart default for the user or do I expose the interface? I went towards exposing the interface... but I do also love smart defaults. Regardless, you are right, if we expose the option it probably should take the double and split it up between seconds and microseconds. Two parenthetical thoughts... a) oddly not milliseconds - I wonder how they are doing their timing such that they imagine they get that level of time resolution ... b) does a user really care about resolution below the second level here?

This is just an initialization step, so it only happens once; so long as it times out before the user feels the need to tap their finger at it and say 'is this thing on?', no problem. The slippery slope argument from there is that perhaps all connection requests should just use redisConnectWithTimeout and we should select a sensible default value for all connections. I think it makes a certain amount of sense to do that, and then perhaps to accept an override value in the constructor if they aren't happy with our default.

@eddelbuettel
Copy link
Owner

@eddelbuettel eddelbuettel commented Oct 6, 2015

I frankly never had the time out problem so 'shrugs'. Wait a min -- editing on train right now.

@eddelbuettel
Copy link
Owner

@eddelbuettel eddelbuettel commented Oct 6, 2015

This should do, right?

    // set up a connection to Redis on the given machine and port
    void init(std::string host="127.0.0.1", int port=6379,
              std::string auth="", double timeout=0.0)  {
        if (timeout == 0) {
            prc_ = redisConnect(host.c_str(), port);
        } else {
            int second = static_cast<int>(timeout);
            int microseconds = static_cast<int>(1000*(timeout - second));
            struct timeval timeoutStruct = { timeout, microseconds };
            prc_ = redisConnectWithTimeout(host.c_str(), port, timeoutStruct);
        }
@russellpierce
Copy link
Contributor Author

@russellpierce russellpierce commented Oct 6, 2015

I think instead use 1000000*(timeout - second). You want microseconds. cf. https://github.com/redis/hiredis/blob/master/examples/example.c

@eddelbuettel
Copy link
Owner

@eddelbuettel eddelbuettel commented Oct 6, 2015

Touche. Will fix.

I was also about to send you there for 'proper' defauls. Does it have any?

eddelbuettel added a commit that referenced this pull request Oct 6, 2015
@russellpierce
Copy link
Contributor Author

@russellpierce russellpierce commented Oct 6, 2015

The example uses 1.5 seconds, but as far as I can tell redisConnectWithTimeout has no default time value.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants
You can’t perform that action at this time.