Skip to content

Conversation

@ghost
Copy link

@ghost ghost commented Dec 16, 2013

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
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
Copy link

cmcavoy commented Dec 18, 2013

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
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
Copy link
Author

ghost commented Dec 18, 2013

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

@ghost
Copy link
Author

ghost commented Dec 19, 2013

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
Copy link
Author

ghost commented Dec 19, 2013

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