Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with
or
.
Download ZIP

Loading…

Ability to setup different values for connection and idle timeouts #93

Merged
merged 1 commit into from

3 participants

@kaero

Commit adds an option idle to set timeout for connection idle different from connection timeout.

@3rd-Eden 3rd-Eden commented on the diff
lib/memcached.js
@@ -158,9 +162,13 @@ Client.config = {
Manager.remove(this);
}
, data: curry(memcached, privates.buffer, S)
- , timeout: function streamTimeout() {
- Manager.remove(this);
+ , connect: function streamConnect() {
+ if (this.memcached.idle > this.memcached.timeout) {
+ this.setTimeout(0, streamTimeout);
@3rd-Eden Owner

What's the reason for 2 setTimeout's?

@kaero
kaero added a note

socket.setTimeout(0, ...) remove the old timeout.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@3rd-Eden 3rd-Eden commented on the diff
lib/memcached.js
@@ -158,9 +162,13 @@ Client.config = {
Manager.remove(this);
}
, data: curry(memcached, privates.buffer, S)
- , timeout: function streamTimeout() {
@3rd-Eden Owner

This looks like useless move to me

@kaero
kaero added a note

Mm.. can you tell me more details, what is looks useless?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@3rd-Eden
Owner

Overall looks good except those 2 issues that i've commented on. Could add a test as well so there will not be any regression of this in the future?

@kaero

Thanks for comments, i'll add a test and attach it to this thread.

@ianshward
Collaborator

I started a branch in my fork https://github.com/ianshward/node-memcached/compare/connect-idle-timeout to merge into latest master so that timeouts before the connect event will be considered connection issues (as in calling connectionIssue) whereas timeouts after the connect event will only be considered idle connections. I'll work on tests next.

@3rd-Eden
Owner

Yeah, having tests for these things is really important as I eventually want to finish the 1.0 branch that i've started a while ago. And I don't want to introduce any regression with this major rewrite.

@kaero

I'm sorry about such long delay, guys. @ianshward are you in needs for help with tests?

@ianshward
Collaborator

@kaero if you have time to write tests today that'd be great. Let me know. Here's your branch merged into latest master, which includes reporting connection issues when the timeout occurs before the connect event https://github.com/ianshward/node-memcached/compare/connect-idle-timeout

@ianshward
Collaborator
@kaero

@ianshward yes, i found and remember this, and remove the comment :)

I'll write tests today.

@ianshward
Collaborator

@kaero great. I just pushed another commit to https://github.com/ianshward/node-memcached/compare/connect-idle-timeout which releases a connection if it timed out before connect, or on error. I see you already had this on idle connection timeouts. I saw that if the connection is not removed from the pool, this.retries in the issue logger would get into a negative state, ie -3, -5, etc, skipping 0 and therefore never reach the code that removes the server.

@ianshward
Collaborator

@kaero have you made any progress on this? I ask because I may have time to do this Friday, some time after 10AM UTC-4, unless you've gotten to it before me.

@kaero

@ianshward I've a problem with timeout and idle options tests. I'm trying to debug them now.

  it('should reset connection on timeout defined by `idle` option', function(done) {
      var mock = net.createServer(function(sock) {
              setTimeout(function() {
                  sock.destroy();
              }, 3000);
          }),
          memcached = new Memcached(['127.0.0.1:11219'], {
              timeout: 3000,
              idle: 1000,
              retries: 0,
              remove: false });

          this.timeout(10000);

          mock.listen(11219);

          memcached.get('y', function(err) {
              memcached.end();
              mock.close();
              done();
          });
    });

Looks like get callback is never called in this code.

My time zone is UTC+4, so i have about 14h :)

@kaero

I'm feeling stupid. Looks like timeouts not working at all. But i see in the debugger how Utils.fuse setup them on socket! T_T

@kaero

Ok, idleTimeout, connectTimeout and streamError callbacks are called actually on associated errors, but their execution doesn't leads to calling callbacks of the memcached get/set methods and they waiting forever.

@kaero

I am almost sure, that there is a design issue. Connection errors can be caught in listeners for issue/failure/remove events of memcached instance.

Though callback, that was passed to get/set methods doesn't fire. Thus client will never know that such an error occured.

To cancel the callback, user must create some kind of workaround. I don't think it's OK.

So my proposal is: in case of connection error, fire a callback passed to get/set methods.

@ianshward
Collaborator

@kaero it looks like there are a few reports that may be related to what you've found:

@kaero

#76 & #96 are completely revelevant and #88 requires further investigation.

@kaero

WIP, i'll publish some results in a hour.

@3rd-Eden
Owner

@kaero You might need to rebase against the current master as this pull request cannot be merged due to conflicts.

@kaero

@3rd-Eden ok, i'll do it, when changes in my branch will be completely done.

@kaero

@ianshward test for connection timeout and a little fix to fire failure event when removed server has no callbacks kaero/node-memcached@ianshward:connect-idle-timeout...connection-issues

Now we must determine where to fire query callback with error. I've tried connectionIssue on the fly, but not successful – other connection tests fails.

@ianshward
Collaborator

@kaero are you trying like this?

        , streamError = function() {
            memcached.connectionIssue('Stream error', S);
            Manager.remove(this);
            memcached.makeCallback(callback, new Error('Stream error...'));
        };

it sounds like this is what you're doing. I believe the existing "multi get multi server" test may actually be broken, in master, and it just results in a timeout b/c the callback is never called. Your fix here w/ calling the callback just makes that test fail instead of timing out, so I don't think your changes actually break that test - it was already broken. I could see how other tests might break now that the callback is properly called, ie, anything that was testing certain timing.

@kaero

@ianshward

I'm trying following:

memcached.connectionIssue('Stream error', S, callback);

and in the connectionIssue before log call:

if (callback) {
    callback(error);
}

I'll try memcached.makeCallback in this place.

So, i'll continue and try to done in one or two days with library code and tests fixes.

@ianshward
Collaborator

@kaero ok great. It looks like you may want to use memcached.makeCallback, because when memcached.command is called (which calls this.connect) activeQueries is incremented which is used to limit the number of allowed active queries, and makeCallback will decrement it.

UPDATE: I totally missed that you already said you're going to try makeCallback, sorry.

@ianshward
Collaborator

@kaero I left an inline comment on your branch, kaero/node-memcached@e4fc192#commitcomment-3676563

@ianshward ianshward merged commit 0cd421e into 3rd-Eden:master
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
This page is out of date. Refresh to see the latest.
Showing with 11 additions and 3 deletions.
  1. +11 −3 lib/memcached.js
View
14 lib/memcached.js
@@ -78,6 +78,7 @@ Client.config = {
, poolSize: 10 // maximal parallel connections
, reconnect: 18000000 // if dead, attempt reconnect each xx ms
, timeout: 5000 // after x ms the server should send a timeout if we can't connect
+ , idle: 5000 // connection idle timeout
, retries: 5 // amount of retries before server is dead
, retry: 30000 // timeout between retries, all call will be marked as cache miss
, remove: false // remove server if dead if false, we will attempt to reconnect
@@ -138,7 +139,10 @@ Client.config = {
var S = Array.isArray(serverTokens)
? new Stream
: new Socket
- , Manager = this;
+ , Manager = this
+ , streamTimeout = function () {
+ Manager.remove(this);
+ };
// config the Stream
S.streamID = sid++;
@@ -158,9 +162,13 @@ Client.config = {
Manager.remove(this);
}
, data: curry(memcached, privates.buffer, S)
- , timeout: function streamTimeout() {
@3rd-Eden Owner

This looks like useless move to me

@kaero
kaero added a note

Mm.. can you tell me more details, what is looks useless?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
- Manager.remove(this);
+ , connect: function streamConnect() {
+ if (this.memcached.idle > this.memcached.timeout) {
+ this.setTimeout(0, streamTimeout);
@3rd-Eden Owner

What's the reason for 2 setTimeout's?

@kaero
kaero added a note

socket.setTimeout(0, ...) remove the old timeout.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
+ this.setTimeout(this.memcached.idle, streamTimeout);
+ }
}
+ , timeout: streamTimeout
, end: S.end
});
Something went wrong with that request. Please try again.