search

brianloveswords / streamsql

A streaming, backend agnostic SQL ORM heavily inspired by levelup
MIT License
67 stars 7 forks source link

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

Open ghost opened 10 years ago

ghost commented 10 years ago

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 commented 10 years ago

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 commented 10 years ago

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 commented 10 years ago

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 commented 10 years ago

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

ghost commented 10 years ago

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 commented 10 years ago

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