{"action":"opened","number":12,"pull_request":{"url":"https://api.github.com/repos/wankdanker/node-odbc/pulls/12","id":30185902,"html_url":"https://github.com/wankdanker/node-odbc/pull/12","diff_url":"https://github.com/wankdanker/node-odbc/pull/12.diff","patch_url":"https://github.com/wankdanker/node-odbc/pull/12.patch","issue_url":"https://api.github.com/repos/wankdanker/node-odbc/issues/12","number":12,"state":"open","locked":false,"title":"Fixed leaking V8 handles through unnecessary HandleScope.Close() calls","user":{"login":"Xylem","id":2742582,"avatar_url":"https://avatars.githubusercontent.com/u/2742582?v=3","gravatar_id":"","url":"https://api.github.com/users/Xylem","html_url":"https://github.com/Xylem","followers_url":"https://api.github.com/users/Xylem/followers","following_url":"https://api.github.com/users/Xylem/following{/other_user}","gists_url":"https://api.github.com/users/Xylem/gists{/gist_id}","starred_url":"https://api.github.com/users/Xylem/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/Xylem/subscriptions","organizations_url":"https://api.github.com/users/Xylem/orgs","repos_url":"https://api.github.com/users/Xylem/repos","events_url":"https://api.github.com/users/Xylem/events{/privacy}","received_events_url":"https://api.github.com/users/Xylem/received_events","type":"User","site_admin":false},"body":"Calling `HandleScope.Close()` is unnecessary when its return value is not returned to a calling function. Additionaly, it causes V8 handles to be leaked and causes major memory leaks over time. The issue becomes apparent mainly in long-running and database-intensive applications.\r\n\r\n### Reproduction\r\n\r\nThe following code is a minimal program allowing for reproduction of this issue. Apart from `odbc`, it uses `async@0.9.0` and `heapdump@0.3.5`. In my case, I tested the issue using an instance of Postgres running on the same VM.\r\n```javascript\r\nvar heapdump = require('heapdump');\r\nvar async = require('async');\r\nvar db = require('odbc')();\r\n\r\ndb.open('DSN=TestDB;SERVER=localhost;UID=postgres;PWD=postgres', function () {\r\n  async.forever(function (done) {\r\n    db.query('select 1', function (err) {\r\n      done(err);\r\n    });\r\n  });\r\n});\r\n```\r\n\r\nAfter running it, V8 heap dumps can be generated by issuing\r\n```\r\nkill -USR2 <PID of node process>\r\n```\r\ncommand.\r\n\r\nWith the version of `odbc` without these changes, the size of heap dumps of the running process should increase ~3-5MB per minute. When [loaded intro Chrome Dev Tools](https://github.com/bnoordhuis/node-heapdump#inspecting-the-snapshot), however, the reported heap size for each of the generated files should not exceed ~2MB. Since the heap dump is simply a JSON file, it can be analyzed manually, which I did. What follows is a description of my analysis of the following heap dump: https://drive.google.com/file/d/0B1gxZN-e6g8ZR09FNGk4TVFVaUE/view?usp=sharing\r\n\r\nIn array `nodes`, close to the beginning of the file, you should be able to see a line similar to the following: `,3,9,23,0,3792103`. The fields, from left to right, are `type` (with `3` being `object`), `name`, (being index in table `strings`, in this case: `(Handle scope)`), `id` of the node, `self_size` of the node and finally, `edge_count` - the number of edges outgoing from that node\r\n\r\nFurther in the file, in the array `edges`, there should be a significant number of lines similar to `,3,3415,80`. The fields, from left to right, are `type` (with `3` being `internal`), `name_or_index` (again, in this case, referencing index from `strings` array - contrary to previous case, the index varies on each run, but it always points to string `undefined_value`) and `to_node` being the `id` of a node it connects to. I don't know the exact format of the heap dump, but I assume that the origin node for the edge is calculated based on ordering of nodes in the `nodes` array and their reported `edge_count`. In case of the heap dump I'm describing, there were, in total, 3792107 edges referencing the `undefined` value, which matches very closely the number of edges outgoing from the `(Handle scope)` node.\r\n\r\nFor comparison, here's a heap dump of the same program after applying my changes: https://drive.google.com/file/d/0B1gxZN-e6g8ZUkNWc1VOUTZ1eFk/view?usp=sharing\r\n\r\n### Cause\r\n\r\n> This part is partially speculation, as I didn't do an in-depth analysis of the V8 innards, but it seems to match the observed behavior\r\n\r\nThe mechanism of `HandleScope`s is based on a stack. When a new `HandleScope` is created, it is put on top of the scope stack. When `Handle<T>` objects are created (as well as `Local<T>`), they are being referenced by the `HandleScope` being currently at the top of the scope stack. Once that `HandleScope` is removed from the scope stack (e.g. when function it was created in as a local variable returns), it takes care of disposing the handles that exist within it. This is why it is unsafe to simply return `Handle<T>` objects from a function - since they belonged to a `HandleScope` that was deleted, anything they pointed to may already not exist. For that reason, the `HandleScope.Close()` function exists. What it does is takes in a handle from the scope being closed and, after removing the scope from the scope stack, creates a new handle to the same object. Since the scope we just closed is not present on the scope stack, the newly created handle will be created within the `HandleScope` further down the stack. Here's an example:\r\n```\r\nHandle<Object> FunctionCalledByJS(...) {\r\n  HandleScope scopeA; // scopeA is put on a scope stack\r\n  Local<Object> objectA; // The Local<Object> handle\r\n                         // is being created within\r\n                         // the scopeA scope\r\n\r\n  Local<Object> objectC = InternalFunction(); --> Handle<Object> InternalFunction() {\r\n                                                    HandleScope scopeB; // scopeB is put on a scope stack\r\n                                                    Local<Object> objectB; // Handle is being created\r\n                                                                           // within the scopeB scope\r\n                                                    return scopeB.Close(objectB); // scopeB is removed from the stack\r\n                                                                                  // new Handle<Object> is created in scopeA\r\n                                                                                  // Local<Object> objectB handle is disposed\r\n                                                                                  // newly created handle is returned to function FunctionCalledByJS\r\n                                              <-- }\r\n  return scopeA.Close(objectA); // scopeA is removed from the stack\r\n                                // currently, there is scope of the calling JavaScript code on top of the stack\r\n                                // new Handle<Object> is created in the scope of JS code\r\n                                // Local<Object> objectA handle is disposed\r\n                                // Local<Object> objectC handle is disposed\r\n                                // newly created handle is returned to JS code\r\n}\r\n```\r\n\r\nNow, in the cases that were causing the issue, the following was being done:\r\n```\r\nvoid VoidFunction(...) {\r\n  HandleScope scope;\r\n\r\n  // code\r\n\r\n  scope.Close(Undefined());\r\n}\r\n```\r\n\r\nAs you can see, the handle created by the `HandleScope.Close()` function is not being returned anywhere. This is particularly problematic with all `UV_After<Operation>` functions, as, I assume, once `UV_<Operation>` function exits and `libuv` calls the `UV_After<Operation>` back in the main application thread, the only scope being present on the handle scope stack is the global handle scope (or other scope that exists throughout the entire lifetime of the process), which causes the handles created by `HandleScope.Close()` to exist until the process is terminated.\r\n\r\nAs for the specifics of the memory leak created by the issue, I'm not entirely sure. Partially, it's definitely information about all those handles being stored. I also suspect that it might be the case that V8's garbage collector is simply being crippled by the sheer amount of handles it has to keep track of to the point that it's no longer able to cope with freeing memory allocated by other objects that, under normal conditions, wouldn't cause problems.\r\n\r\n### Solution\r\n\r\nI removed all calls to `HandleScope.Close()` where the return value was unused. There are no observable ill effects to these changes.","created_at":"2015-02-27T17:46:53Z","updated_at":"2015-02-27T17:46:53Z","closed_at":null,"merged_at":null,"merge_commit_sha":null,"assignee":null,"milestone":null,"commits_url":"https://api.github.com/repos/wankdanker/node-odbc/pulls/12/commits","review_comments_url":"https://api.github.com/repos/wankdanker/node-odbc/pulls/12/comments","review_comment_url":"https://api.github.com/repos/wankdanker/node-odbc/pulls/comments{/number}","comments_url":"https://api.github.com/repos/wankdanker/node-odbc/issues/12/comments","statuses_url":"https://api.github.com/repos/wankdanker/node-odbc/statuses/52f918e2ade60371c3a28312d8023006eadf8340","head":{"label":"Xylem:master","ref":"master","sha":"52f918e2ade60371c3a28312d8023006eadf8340","user":{"login":"Xylem","id":2742582,"avatar_url":"https://avatars.githubusercontent.com/u/2742582?v=3","gravatar_id":"","url":"https://api.github.com/users/Xylem","html_url":"https://github.com/Xylem","followers_url":"https://api.github.com/users/Xylem/followers","following_url":"https://api.github.com/users/Xylem/following{/other_user}","gists_url":"https://api.github.com/users/Xylem/gists{/gist_id}","starred_url":"https://api.github.com/users/Xylem/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/Xylem/subscriptions","organizations_url":"https://api.github.com/users/Xylem/orgs","repos_url":"https://api.github.com/users/Xylem/repos","events_url":"https://api.github.com/users/Xylem/events{/privacy}","received_events_url":"https://api.github.com/users/Xylem/received_events","type":"User","site_admin":false},"repo":{"id":31427197,"name":"node-odbc","full_name":"Xylem/node-odbc","owner":{"login":"Xylem","id":2742582,"avatar_url":"https://avatars.githubusercontent.com/u/2742582?v=3","gravatar_id":"","url":"https://api.github.com/users/Xylem","html_url":"https://github.com/Xylem","followers_url":"https://api.github.com/users/Xylem/followers","following_url":"https://api.github.com/users/Xylem/following{/other_user}","gists_url":"https://api.github.com/users/Xylem/gists{/gist_id}","starred_url":"https://api.github.com/users/Xylem/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/Xylem/subscriptions","organizations_url":"https://api.github.com/users/Xylem/orgs","repos_url":"https://api.github.com/users/Xylem/repos","events_url":"https://api.github.com/users/Xylem/events{/privacy}","received_events_url":"https://api.github.com/users/Xylem/received_events","type":"User","site_admin":false},"private":false,"html_url":"https://github.com/Xylem/node-odbc","description":"unixODBC bindings for node","fork":true,"url":"https://api.github.com/repos/Xylem/node-odbc","forks_url":"https://api.github.com/repos/Xylem/node-odbc/forks","keys_url":"https://api.github.com/repos/Xylem/node-odbc/keys{/key_id}","collaborators_url":"https://api.github.com/repos/Xylem/node-odbc/collaborators{/collaborator}","teams_url":"https://api.github.com/repos/Xylem/node-odbc/teams","hooks_url":"https://api.github.com/repos/Xylem/node-odbc/hooks","issue_events_url":"https://api.github.com/repos/Xylem/node-odbc/issues/events{/number}","events_url":"https://api.github.com/repos/Xylem/node-odbc/events","assignees_url":"https://api.github.com/repos/Xylem/node-odbc/assignees{/user}","branches_url":"https://api.github.com/repos/Xylem/node-odbc/branches{/branch}","tags_url":"https://api.github.com/repos/Xylem/node-odbc/tags","blobs_url":"https://api.github.com/repos/Xylem/node-odbc/git/blobs{/sha}","git_tags_url":"https://api.github.com/repos/Xylem/node-odbc/git/tags{/sha}","git_refs_url":"https://api.github.com/repos/Xylem/node-odbc/git/refs{/sha}","trees_url":"https://api.github.com/repos/Xylem/node-odbc/git/trees{/sha}","statuses_url":"https://api.github.com/repos/Xylem/node-odbc/statuses/{sha}","languages_url":"https://api.github.com/repos/Xylem/node-odbc/languages","stargazers_url":"https://api.github.com/repos/Xylem/node-odbc/stargazers","contributors_url":"https://api.github.com/repos/Xylem/node-odbc/contributors","subscribers_url":"https://api.github.com/repos/Xylem/node-odbc/subscribers","subscription_url":"https://api.github.com/repos/Xylem/node-odbc/subscription","commits_url":"https://api.github.com/repos/Xylem/node-odbc/commits{/sha}","git_commits_url":"https://api.github.com/repos/Xylem/node-odbc/git/commits{/sha}","comments_url":"https://api.github.com/repos/Xylem/node-odbc/comments{/number}","issue_comment_url":"https://api.github.com/repos/Xylem/node-odbc/issues/comments{/number}","contents_url":"https://api.github.com/repos/Xylem/node-odbc/contents/{+path}","compare_url":"https://api.github.com/repos/Xylem/node-odbc/compare/{base}...{head}","merges_url":"https://api.github.com/repos/Xylem/node-odbc/merges","archive_url":"https://api.github.com/repos/Xylem/node-odbc/{archive_format}{/ref}","downloads_url":"https://api.github.com/repos/Xylem/node-odbc/downloads","issues_url":"https://api.github.com/repos/Xylem/node-odbc/issues{/number}","pulls_url":"https://api.github.com/repos/Xylem/node-odbc/pulls{/number}","milestones_url":"https://api.github.com/repos/Xylem/node-odbc/milestones{/number}","notifications_url":"https://api.github.com/repos/Xylem/node-odbc/notifications{?since,all,participating}","labels_url":"https://api.github.com/repos/Xylem/node-odbc/labels{/name}","releases_url":"https://api.github.com/repos/Xylem/node-odbc/releases{/id}","created_at":"2015-02-27T16:03:09Z","updated_at":"2015-02-27T16:20:15Z","pushed_at":"2015-02-27T16:20:14Z","git_url":"git://github.com/Xylem/node-odbc.git","ssh_url":"git@github.com:Xylem/node-odbc.git","clone_url":"https://github.com/Xylem/node-odbc.git","svn_url":"https://github.com/Xylem/node-odbc","homepage":"","size":1816,"stargazers_count":0,"watchers_count":0,"language":"C++","has_issues":false,"has_downloads":true,"has_wiki":true,"has_pages":false,"forks_count":0,"mirror_url":null,"open_issues_count":0,"forks":0,"open_issues":0,"watchers":0,"default_branch":"master"}},"base":{"label":"wankdanker:master","ref":"master","sha":"c0316d785a478ea30a9fd80c95d4f253c89ebd1e","user":{"login":"wankdanker","id":624233,"avatar_url":"https://avatars.githubusercontent.com/u/624233?v=3","gravatar_id":"","url":"https://api.github.com/users/wankdanker","html_url":"https://github.com/wankdanker","followers_url":"https://api.github.com/users/wankdanker/followers","following_url":"https://api.github.com/users/wankdanker/following{/other_user}","gists_url":"https://api.github.com/users/wankdanker/gists{/gist_id}","starred_url":"https://api.github.com/users/wankdanker/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/wankdanker/subscriptions","organizations_url":"https://api.github.com/users/wankdanker/orgs","repos_url":"https://api.github.com/users/wankdanker/repos","events_url":"https://api.github.com/users/wankdanker/events{/privacy}","received_events_url":"https://api.github.com/users/wankdanker/received_events","type":"User","site_admin":false},"repo":{"id":1384387,"name":"node-odbc","full_name":"wankdanker/node-odbc","owner":{"login":"wankdanker","id":624233,"avatar_url":"https://avatars.githubusercontent.com/u/624233?v=3","gravatar_id":"","url":"https://api.github.com/users/wankdanker","html_url":"https://github.com/wankdanker","followers_url":"https://api.github.com/users/wankdanker/followers","following_url":"https://api.github.com/users/wankdanker/following{/other_user}","gists_url":"https://api.github.com/users/wankdanker/gists{/gist_id}","starred_url":"https://api.github.com/users/wankdanker/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/wankdanker/subscriptions","organizations_url":"https://api.github.com/users/wankdanker/orgs","repos_url":"https://api.github.com/users/wankdanker/repos","events_url":"https://api.github.com/users/wankdanker/events{/privacy}","received_events_url":"https://api.github.com/users/wankdanker/received_events","type":"User","site_admin":false},"private":false,"html_url":"https://github.com/wankdanker/node-odbc","description":"unixODBC bindings for node","fork":true,"url":"https://api.github.com/repos/wankdanker/node-odbc","forks_url":"https://api.github.com/repos/wankdanker/node-odbc/forks","keys_url":"https://api.github.com/repos/wankdanker/node-odbc/keys{/key_id}","collaborators_url":"https://api.github.com/repos/wankdanker/node-odbc/collaborators{/collaborator}","teams_url":"https://api.github.com/repos/wankdanker/node-odbc/teams","hooks_url":"https://api.github.com/repos/wankdanker/node-odbc/hooks","issue_events_url":"https://api.github.com/repos/wankdanker/node-odbc/issues/events{/number}","events_url":"https://api.github.com/repos/wankdanker/node-odbc/events","assignees_url":"https://api.github.com/repos/wankdanker/node-odbc/assignees{/user}","branches_url":"https://api.github.com/repos/wankdanker/node-odbc/branches{/branch}","tags_url":"https://api.github.com/repos/wankdanker/node-odbc/tags","blobs_url":"https://api.github.com/repos/wankdanker/node-odbc/git/blobs{/sha}","git_tags_url":"https://api.github.com/repos/wankdanker/node-odbc/git/tags{/sha}","git_refs_url":"https://api.github.com/repos/wankdanker/node-odbc/git/refs{/sha}","trees_url":"https://api.github.com/repos/wankdanker/node-odbc/git/trees{/sha}","statuses_url":"https://api.github.com/repos/wankdanker/node-odbc/statuses/{sha}","languages_url":"https://api.github.com/repos/wankdanker/node-odbc/languages","stargazers_url":"https://api.github.com/repos/wankdanker/node-odbc/stargazers","contributors_url":"https://api.github.com/repos/wankdanker/node-odbc/contributors","subscribers_url":"https://api.github.com/repos/wankdanker/node-odbc/subscribers","subscription_url":"https://api.github.com/repos/wankdanker/node-odbc/subscription","commits_url":"https://api.github.com/repos/wankdanker/node-odbc/commits{/sha}","git_commits_url":"https://api.github.com/repos/wankdanker/node-odbc/git/commits{/sha}","comments_url":"https://api.github.com/repos/wankdanker/node-odbc/comments{/number}","issue_comment_url":"https://api.github.com/repos/wankdanker/node-odbc/issues/comments{/number}","contents_url":"https://api.github.com/repos/wankdanker/node-odbc/contents/{+path}","compare_url":"https://api.github.com/repos/wankdanker/node-odbc/compare/{base}...{head}","merges_url":"https://api.github.com/repos/wankdanker/node-odbc/merges","archive_url":"https://api.github.com/repos/wankdanker/node-odbc/{archive_format}{/ref}","downloads_url":"https://api.github.com/repos/wankdanker/node-odbc/downloads","issues_url":"https://api.github.com/repos/wankdanker/node-odbc/issues{/number}","pulls_url":"https://api.github.com/repos/wankdanker/node-odbc/pulls{/number}","milestones_url":"https://api.github.com/repos/wankdanker/node-odbc/milestones{/number}","notifications_url":"https://api.github.com/repos/wankdanker/node-odbc/notifications{?since,all,participating}","labels_url":"https://api.github.com/repos/wankdanker/node-odbc/labels{/name}","releases_url":"https://api.github.com/repos/wankdanker/node-odbc/releases{/id}","created_at":"2011-02-18T22:09:53Z","updated_at":"2015-02-23T22:36:32Z","pushed_at":"2014-09-30T15:09:13Z","git_url":"git://github.com/wankdanker/node-odbc.git","ssh_url":"git@github.com:wankdanker/node-odbc.git","clone_url":"https://github.com/wankdanker/node-odbc.git","svn_url":"https://github.com/wankdanker/node-odbc","homepage":"","size":1816,"stargazers_count":76,"watchers_count":76,"language":"C++","has_issues":false,"has_downloads":true,"has_wiki":true,"has_pages":false,"forks_count":22,"mirror_url":null,"open_issues_count":4,"forks":22,"open_issues":4,"watchers":76,"default_branch":"master"}},"_links":{"self":{"href":"https://api.github.com/repos/wankdanker/node-odbc/pulls/12"},"html":{"href":"https://github.com/wankdanker/node-odbc/pull/12"},"issue":{"href":"https://api.github.com/repos/wankdanker/node-odbc/issues/12"},"comments":{"href":"https://api.github.com/repos/wankdanker/node-odbc/issues/12/comments"},"review_comments":{"href":"https://api.github.com/repos/wankdanker/node-odbc/pulls/12/comments"},"review_comment":{"href":"https://api.github.com/repos/wankdanker/node-odbc/pulls/comments{/number}"},"commits":{"href":"https://api.github.com/repos/wankdanker/node-odbc/pulls/12/commits"},"statuses":{"href":"https://api.github.com/repos/wankdanker/node-odbc/statuses/52f918e2ade60371c3a28312d8023006eadf8340"}},"merged":false,"mergeable":null,"mergeable_state":"unknown","merged_by":null,"comments":0,"review_comments":0,"commits":1,"additions":1,"deletions":33,"changed_files":4}}