Fix issue #154 - mysql server closing socket causes connection to be unusable #155
Conversation
terminated the connection. In general resetting the connection now resets all pending states that are moot.
| // clear all statements, and reset for using again. | ||
| private void clear() | ||
| { | ||
| if(ids.length) |
There was a problem hiding this comment.
This is an optimization, you should avoid calling assumeSafeAppend if you can, as it isn't cheap.
There was a problem hiding this comment.
I was more concerned about minimizing GC heap usage, but if assumeSafeAppend is expensive, it probably wouldn't hurt to change ids to a more suitable datatype anyway (although not in this PR, of course).
There was a problem hiding this comment.
it's not super-expensive, but it's certainly more expensive than a check for 0 length. It's an opaque call, which may possibly take the GC lock, and even when it doesn't, it's looking through a cache to find the block information.
Fastest solution would be an additional "used" field that stores how many ids are in the list, and then you don't depend on the runtime or opaque calls at all unless you need to expand.
But this was just such a low hanging fruit that I couldn't help picking it :)
schveiguy
changed the title
|
Note: I tested locally, and I ran the same test against the prior version of the code to verify it fails. So I think the test is good, let me know if I did it wrong. And damn you Nick and your stupid tab spacing 😉 |
| purgeResult(); | ||
| statementsToRelease.releaseAll(); | ||
| } | ||
| catch(Exception e) |
There was a problem hiding this comment.
What exception exactly were you getting? It would be nice to be able to restrict this exception squelching to the cases where it is indeed a dead connection. However, as that may not be especially straightforward to figure out right now, I don't think that's enough to delay this PR and v1.2.2.
Everything else looks good to me.
There was a problem hiding this comment.
What exception exactly were you getting?
I thought the same thing when I started to go this route. So I did:
writeln(typeid(e).name);The answer was: object.Exception 😆
|
Tagged v1.2.2 |
|
Thanks! |
In my use cases, I'm leaving open the connection to the mysql server to avoid having to do the connection handshake for every transaction.
However, periodically, the mysql server will close the connection, leaving the application to figure out how to deal with it.
The current code will kill the connection, but this simply closes the socket (which is already closed), and does not actually reset any other state. In particular, deferred cleanup via autoPurge is not reset.
In this update, I'm resetting all cleanup state in the kill function, and ignoring any exceptions during autoPurge. Ignoring all exceptions is OK, because we are going to kill the connection if this happens, and if it does, the server is going to clean up the mess for us (release any prepared statements, and flush any unread data).