Unhandled Exception in connection.end() for MySQL Dialect

[I00000]

fix for #1798

@eelhigu: please review?

[elhigu]

I'm on my mobile so I cannot be browsing the code right now to verify if this is ok... do you know why that removeAllListeners call is there and will there be event listeners left if its not called anymore? @wubzz do you know anything about this?

[I00000]

hi @tgriesser,

In your following commit 7069ce5#diff-b0704a0e57b09b0c3670a2c6b953da1d, you have added that connection.removeAllListeners(), do you know why because in my scenario, it crashes my server and I don't really understand why this layer is responsible for cleaning all listeners.

Thanks in advance for your clarification

[wubzz]

@elhigu @I00000 Sorry for the slow response.. I know we've had issues about this in the past, specifically for mysql. I'm not sure why .removeAllListeners() was added tbh, I'd think calling .end() makes this call redundant. Only @tgriesser would know the answer to this, so I can't really make a decision here.

However, I don't quite understand why the following would not work:

connection.end((err) => {
       connection.removeAllListeners();
        if (err) connection.__knex__disposed = err
});

What error remains after this change? Since the event listeners aren't removed until after .end() has been called, surely it should work..

I'm also inclined to agree with #1691 reference to the Node docs:
Note that it is bad practice to remove listeners added elsewhere in the code

[I00000]

@wubzz: node crahes

[novemberborn]

I think the problem is that mysql2 transitions connections to a broken state, and the dialect in Knex does not recognize this. #2175 seems to fix it for me.

[elhigu]

Closing in favor of #2175