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

New queries hang when a client connection is closed after the 100 ms period of inactivity #1244

Merged
merged 2 commits into from Jan 14, 2014

Conversation

3 participants
@iuriaranda
Contributor

iuriaranda commented Jan 13, 2014

I'm experiencing some issues with queries that hang up randomly. I'm developing a RESTful API with sequelize (using MySQL) and until now I have not experienced any problems at all. Last month we updated sequelize to the latest stable version (1.6.0) and now when I run our API test suite (which is predictable and constant), there is a specific test case that hangs up sometimes. I've tracked down the issue and I think I've found the problem (or problems).

The thing is that sequelize closes a connection after 100 milliseconds of inactivity, all good so far. But when a query is issued after that 100 ms period of inactivity, it happens to be sent to the mysql module after calling the client.end() but before the callback is called in the connector-manager.js file.

client.end(function() {
  if (!self.useQueue) {
    return client.destroy();
  }

  var intervalObj = null
  var cleanup = function () {
    var retryCt = 0
    // make sure to let client finish before calling destroy
    if (self && self.hasQueuedItems) {
      return
    }
    // needed to prevent mysql connection leak
    client.destroy()
    if (self && self.client) {
      self.client = null
    }
    clearInterval(intervalObj)
  }
  intervalObj = setInterval(cleanup, 10)
  cleanup()
  return
})

The thing is that in this short period of time a query may be issued and the client instance is still available in the connection-manager, but it is already closed to new queries. In this situation, the mysql module emits an error in the run query method, but due to the fact that all the process since the query is issued until the run method is called is synchronous, the query issuer doesn't get the change to subscribe to the query event emitter, so there is a "hang up" effect.

This only happens when pooling is disabled.
I've included a test case and a possible fix.

iuriaranda added some commits Jan 11, 2014

Add testcase to test connection hang ups when a query is issued 100 m…
…s after the last one (right when the client connection is closing)

Also added a possible fix

@ghost ghost assigned mickhansen Jan 13, 2014

@mickhansen

This comment has been minimized.

Contributor

mickhansen commented Jan 13, 2014

Thanks for the PR, i'll go through it tonight.
The test fails on every run without the fix?

@iuriaranda

This comment has been minimized.

Contributor

iuriaranda commented Jan 13, 2014

I've run the test several times and it fails in 99% of the runs (in my machine).

@mickhansen

This comment has been minimized.

Contributor

mickhansen commented Jan 13, 2014

Great :)

I'll try running the test a few times on my own machine and go through your PR - It might take a bit before we merge, always iffy about fiddling with connection managers.

@iuriaranda

This comment has been minimized.

Contributor

iuriaranda commented Jan 13, 2014

Ok, I understand :)

@mickhansen

This comment has been minimized.

Contributor

mickhansen commented Jan 13, 2014

Hmm, the test does not ever fail for me.

@mickhansen

This comment has been minimized.

Contributor

mickhansen commented Jan 13, 2014

But the changes seem well reasoned. @janmeier thoughts?

@iuriaranda

This comment has been minimized.

Contributor

iuriaranda commented Jan 13, 2014

Hmm, weird. I'm not in front of my computer right now, but I'll check it out tomorrow.
The test case creates a new sequelize instance with pooling disabled, but can you check if it is in fact disabled? Just in case...

@mickhansen

This comment has been minimized.

Contributor

mickhansen commented Jan 13, 2014

Thought of that and did check the settings, pooling was turned off correctly. But a whole slew of timing issues could be the issue i suppose.

@janmeier

This comment has been minimized.

Member

janmeier commented Jan 14, 2014

Seems well reasoned yeah, I say we merge

@mickhansen

This comment has been minimized.

Contributor

mickhansen commented Jan 14, 2014

Definietely makes sense that the client should be made unavailable for further queries when the disconnect sequence starts.

mickhansen added a commit that referenced this pull request Jan 14, 2014

Merge pull request #1244 from iuriaranda/hotfix/query_hangup
New queries hang when a client connection is closed after the 100 ms period of inactivity

@mickhansen mickhansen merged commit 9f07a7a into sequelize:master Jan 14, 2014

1 check passed

default The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment