Skip to content

[WIP] Modify mysql driver to reconnect after timeout#6

Open
ghost wants to merge 3 commits into
brianloveswords:masterfrom
christensenep:mysql-reconnect
Open

[WIP] Modify mysql driver to reconnect after timeout#6
ghost wants to merge 3 commits into
brianloveswords:masterfrom
christensenep:mysql-reconnect

Conversation

@ghost

@ghost ghost commented Dec 16, 2013

Copy link
Copy Markdown

This has been modified to use pooled connections to solve the timeout problem. Note that there is one significant problem with this PR, namely that it is broken and doesn't actually work.

Well, ok, it works if you set the connectionLimit of the pool to a very high number and make sure not to have too many hasMany relationships in your queries. However, it's possible for the connection pool to run out of connections while handling a single streaming query, since the readStream ends up calling the query() function several times for a given request in some cases. There's currently no way to tell the table.get() function to use a particular connection, so it happily (gleefully, even) gobbles up new connections with each request, eventually deadlocking everything and making me very sad.

I'm working on fixing this now, but I figured I'd updated this PR in the meantime to give you an idea of what things look like so far.

@brianloveswords

Copy link
Copy Markdown
Owner

Hey, could you try to throw together a test for this? Maybe by having a setTimeout with a connection.instance.emit('error', <insert error here>)

@cmcavoy

cmcavoy commented Dec 18, 2013

Copy link
Copy Markdown

Reconnecting on a connect error makes sense...is it possible to use the node-mysql connection pooling feature instead? According to the doc, it handles reopening connections.

@brianloveswords

Copy link
Copy Markdown
Owner

That would be super rad, connection pooling would definitely be the best way to handle this (as long as it's not a rabbit hole)

@ghost

ghost commented Dec 18, 2013

Copy link
Copy Markdown
Author

I had the same thought about connection pooling right after I initially busted this PR, so I will give that a shot.

@ghost

ghost commented Dec 19, 2013

Copy link
Copy Markdown
Author

Oh, also, one of the test failures is due to the readStream now returning rows in unsorted orders, which is again due to the readStream using tons of different connections, rather than a single connection guaranteed to return results in order.

There also appears to be another failure with node 0.8. Not sure what's up with that yet.

@ghost

ghost commented Dec 19, 2013

Copy link
Copy Markdown
Author

Edited the PR description to describe the new pooled connection approach.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants