{"action":"created","issue":{"url":"https://api.github.com/repos/mscdex/node-mariasql/issues/134","repository_url":"https://api.github.com/repos/mscdex/node-mariasql","labels_url":"https://api.github.com/repos/mscdex/node-mariasql/issues/134/labels{/name}","comments_url":"https://api.github.com/repos/mscdex/node-mariasql/issues/134/comments","events_url":"https://api.github.com/repos/mscdex/node-mariasql/issues/134/events","html_url":"https://github.com/mscdex/node-mariasql/pull/134","id":135478726,"number":134,"title":"fix corrupted big querys (> 70K + much concurrency)","user":{"login":"not-implemented","id":1215371,"avatar_url":"https://avatars.githubusercontent.com/u/1215371?v=3","gravatar_id":"","url":"https://api.github.com/users/not-implemented","html_url":"https://github.com/not-implemented","followers_url":"https://api.github.com/users/not-implemented/followers","following_url":"https://api.github.com/users/not-implemented/following{/other_user}","gists_url":"https://api.github.com/users/not-implemented/gists{/gist_id}","starred_url":"https://api.github.com/users/not-implemented/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/not-implemented/subscriptions","organizations_url":"https://api.github.com/users/not-implemented/orgs","repos_url":"https://api.github.com/users/not-implemented/repos","events_url":"https://api.github.com/users/not-implemented/events{/privacy}","received_events_url":"https://api.github.com/users/not-implemented/received_events","type":"User","site_admin":false},"labels":[],"state":"open","locked":false,"assignee":null,"milestone":null,"comments":3,"created_at":"2016-02-22T17:08:07Z","updated_at":"2016-02-22T18:16:28Z","closed_at":null,"pull_request":{"url":"https://api.github.com/repos/mscdex/node-mariasql/pulls/134","html_url":"https://github.com/mscdex/node-mariasql/pull/134","diff_url":"https://github.com/mscdex/node-mariasql/pull/134.diff","patch_url":"https://github.com/mscdex/node-mariasql/pull/134.patch"},"body":"We got some random strange syntax errors from MySQL server with bigger querys. The error was only reproducable when there were a lot of concurrent operations. When issuing just this one big query, it always worked.\r\n\r\nTCP-Dump showed that the ~100 KiB query was transmitted fine until ~79 KiB ... and then garbage followed, so the server error was correct.\r\n\r\nAfter looking at the code, we noticed that `mysql_real_query_start()` is given the pointer `cur_query` ... but the underlying string was initialized before **on local stack** with `Nan::Utf8String query_s(info[0]);` ...\r\n\r\nThe documentation of `mysql_real_query_start()` is not clear about this, but is _seems_ (I didn't check this) that it does not copy the query again, but just uses the pointer internally for subsequent `mysql_real_query_cont()` calls. So it is not guaranteed that the query is still in memory then (depending on v8 garbage collection). As I don't know the internals of v8 garbage collection, I didn't even try to implement a stable test case for that :-)\r\n\r\nSo we tried to implement a reference to `cur_query` as `cur_query_ref` class member (instead of local `query_s`) and free it after `mysql_real_query_start/cont()` is complete (and in class destructor). So we avoided to create an additional copy of the query every time.\r\n\r\nWith this patch the problem was not reproducable anymore.\r\n\r\nBut - please check if there are still conditions where this patch will produce memory leaks."},"comment":{"url":"https://api.github.com/repos/mscdex/node-mariasql/issues/comments/187302337","html_url":"https://github.com/mscdex/node-mariasql/pull/134#issuecomment-187302337","issue_url":"https://api.github.com/repos/mscdex/node-mariasql/issues/134","id":187302337,"user":{"login":"mscdex","id":54666,"avatar_url":"https://avatars.githubusercontent.com/u/54666?v=3","gravatar_id":"","url":"https://api.github.com/users/mscdex","html_url":"https://github.com/mscdex","followers_url":"https://api.github.com/users/mscdex/followers","following_url":"https://api.github.com/users/mscdex/following{/other_user}","gists_url":"https://api.github.com/users/mscdex/gists{/gist_id}","starred_url":"https://api.github.com/users/mscdex/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/mscdex/subscriptions","organizations_url":"https://api.github.com/users/mscdex/orgs","repos_url":"https://api.github.com/users/mscdex/repos","events_url":"https://api.github.com/users/mscdex/events{/privacy}","received_events_url":"https://api.github.com/users/mscdex/received_events","type":"User","site_admin":false},"created_at":"2016-02-22T18:16:28Z","updated_at":"2016-02-22T18:16:28Z","body":"The `new Nan::Utf8String()` is [already doing an allocation on large strings followed by a copy](https://github.com/nodejs/nan/blob/baa8436ed26cb2f7b2494a12d19befc38d19d612/nan.h#L898-L905), so you would have already noticed a performance difference if there was one.\r\n\r\nI suppose if we wanted to avoid copying the string at all, we could replace the `cur_query` variable entirely with a `Persistent<String>` variable, assign `info[0]` to it in `query()`, and clear it on query finish/error/etc."}}