Added test to trigger value out of bounds bug

[lillem4n]

Referenced to #248 - More commits to come

[sushantdhiman]

wrapping it to try catch feels dirty to me, may be it can return error in some other way, a bit clean approach? I think right now we should solve actual cause as suggested in #248 (comment)

[lillem4n]

I do not really like try/catches either, but I do not know another safe way to make sure no throwed errors are breaking the application implementing this library.

Maybe accept that bugs like this WILL break the app and that's that?

[sidorares]

@lillem4n unfortunately try/catch are expensive and often cause v8 to deoptimize functions. As a workaround for your particular case we could add check to packet length and if > 16M propagate error to callback, until big packet support is landed

[lillem4n]

As long as the library does not throw errors on async calls, I'm perfectly happy. :)

I'm not good enough with this library to make a good enough solution that returns an error, since I do not fully understand the consequences of fiddling with the inside of this.next().

I might dive deeper, but not today.

Hope this helped with showing a bit on what I need on the "client" side.

[sidorares]

@lillem4n thanks a lot for your input, I do share the same - async system library under no circumstance can throw error in a way client is unable to catch. I'll try to evaluate other approaches, maybe try/catch is indeed the only safe solution, even at performance cost

[sidorares]

@lillem4n updates:

1. main issue should be fixed by Add support for reading and writing packets over 16m #421 , we will try to land it first
2. I'll do benchmarking with try/catch at top level of sync code paths. Based on performance cost we might add (or not) them

I'll keep this PR open as placeholder for "how to avoid crashes that are not possible to handle in user code" discussion, but pr itself is unlikely to be merged as is (your test though went to #421)

[sidorares]

try/catch performance research:

while V8 with torbofan does some optimisations for try/catch ( http://v8project.blogspot.com.au/2015/07/v8-45-release.html ) on my simple tests there was significant (up to 10x) penalty for a function with try/catch block, even when try catch is around loop (not inside loop / hot path). Also see https://github.com/petkaantonov/bluebird/wiki/Optimization-killers#2-unsupported-syntax

I did very simple benchmark ( 100k rows from real production data copy - 51 field in a row) and it seems that in practice it's not that bad, ~3% decrease while having 5% std deviation (so it's less than statistical error)

current mysql2:
Milliseconds: 2091 rows per sec: 47824.007651841224 fields per sec: 2439024.3902439023
Average fields per sec over ~30 runs, standard deviation: 2464686, 113277

mysql2 + try/catch around packetParser.execute() only
Milliseconds: 1967 rows per sec: 50838.84087442806 fields per sec: 2592780.8845958314
Average fields per sec over ~30 runs, standard deviation: 2545764 102012

mysql:
Milliseconds: 7957 rows per sec: 12567.550584391101 fields per sec: 640945.0798039462
Average fields per sec over ~30 runs, standard deviation: 667522 191383

mysql is 3.8 times slower at 100k rows, crashing with memory error at 150k ( mysql2 doing fine with full results, 449800 rows)

obviously this is some extreme test where mysql2 is doing especially well ( very large result set ). When you query 1-5 rows at a time perf difference is not so huge

[lillem4n]

@sidorares Thats great! Does that mean try/catch is actually an option after all?

[sidorares]

Does that mean try/catch is actually an option after all?

probably yes. Note that wrapping packetParser.execute() should be enough as it does result callback synchronously (which in turn makes all protocol parsing code up to query/execute callback). Exception in packets or commands (as in /lib/packets/*, /lib/commands/* ) would be caught by this try/catch (unless compression is used, because compression leaves execution flow to event loop here