{"action":"created","issue":{"url":"https://api.github.com/repos/polotek/libxmljs/issues/296","labels_url":"https://api.github.com/repos/polotek/libxmljs/issues/296/labels{/name}","comments_url":"https://api.github.com/repos/polotek/libxmljs/issues/296/comments","events_url":"https://api.github.com/repos/polotek/libxmljs/issues/296/events","html_url":"https://github.com/polotek/libxmljs/issues/296","id":66198239,"number":296,"title":"Async use of NanAdjustExternalMemory causes crash with Node 0.12.1","user":{"login":"gagern","id":190759,"avatar_url":"https://avatars.githubusercontent.com/u/190759?v=3","gravatar_id":"","url":"https://api.github.com/users/gagern","html_url":"https://github.com/gagern","followers_url":"https://api.github.com/users/gagern/followers","following_url":"https://api.github.com/users/gagern/following{/other_user}","gists_url":"https://api.github.com/users/gagern/gists{/gist_id}","starred_url":"https://api.github.com/users/gagern/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/gagern/subscriptions","organizations_url":"https://api.github.com/users/gagern/orgs","repos_url":"https://api.github.com/users/gagern/repos","events_url":"https://api.github.com/users/gagern/events{/privacy}","received_events_url":"https://api.github.com/users/gagern/received_events","type":"User","site_admin":false},"labels":[],"state":"open","locked":false,"assignee":null,"milestone":null,"comments":24,"created_at":"2015-04-03T17:52:48Z","updated_at":"2015-05-29T08:40:07Z","closed_at":null,"body":"I'm using node 0.12.1 and am investigating a segmentation fault in node-libxslt which builds on libxmljs. See albanm/node-libxslt#9.\r\n\r\nI found that the segmentation fault occurs in a worker thread (i.e. not the V8 event dispatcher thread) when that thread runs some libxml code, and as a consequence calls `NanAdjustExternalMemory` at some point. In that thread, `v8::Isolate::GetCurrent()` will return `NULL`, causing the crash. The way I understand it, calls to V8 API from threads other than the event dispatcher thread are forbidden, including that Nan call.\r\n\r\nUnfortunately I still have to find a good documentation on that `AdjustAmountOfExternalAllocatedMemory` method of the isolate, to see what repercussions we'd have to expect if we simply stopped telling V8 about our memory usage. But I guess 393b44a0010da5a469222291e9cad6be74f33a4c introduced that code for a reason. That commit is titled “first round of fixes for external memory issues” but doesn't say what kinds of issues these are. @polotek, do you remember any details?\r\n\r\nIf there is a really strong reason to keep these functions, then we'll probably have to work out how to make them function in an asynchroneous world. Implement some libuv signalling which reports adjustments from worker threads back to the event dispatch thread to be fed to V8. I very much hope that we don't really need this, since it might add considerable synchronization overhead."},"comment":{"url":"https://api.github.com/repos/polotek/libxmljs/issues/comments/106745988","html_url":"https://github.com/polotek/libxmljs/issues/296#issuecomment-106745988","issue_url":"https://api.github.com/repos/polotek/libxmljs/issues/296","id":106745988,"user":{"login":"gagern","id":190759,"avatar_url":"https://avatars.githubusercontent.com/u/190759?v=3","gravatar_id":"","url":"https://api.github.com/users/gagern","html_url":"https://github.com/gagern","followers_url":"https://api.github.com/users/gagern/followers","following_url":"https://api.github.com/users/gagern/following{/other_user}","gists_url":"https://api.github.com/users/gagern/gists{/gist_id}","starred_url":"https://api.github.com/users/gagern/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/gagern/subscriptions","organizations_url":"https://api.github.com/users/gagern/orgs","repos_url":"https://api.github.com/users/gagern/repos","events_url":"https://api.github.com/users/gagern/events{/privacy}","received_events_url":"https://api.github.com/users/gagern/received_events","type":"User","site_admin":false},"created_at":"2015-05-29T08:40:07Z","updated_at":"2015-05-29T08:40:07Z","body":"As far as I can tell, both #300 and #302 are ready for review and inclusion, and either one would get the job done. #302 requires less cooperation from libraries building on this, but appears to come with a CPU time overhead at least when the main loop is idle.\r\n\r\nI consider the amount of cooperation required by #300 acceptable and indeed useful for future changes. I think it's also packaged in such a way that standard C++ RAII will take care of everything with very few extra lines of code. But I'm hardly objective when judging my own APIs.\r\n\r\nNathan considers his code easier to understand, and while I'm not sure I agree, again I'm not objective in that respect. At the moment the patch footprint looks not very much different: +177 -62 for #300 and +155 -60 for #302. The latter contains some libuv compat code which might go away eventually, while the former adds a `libxml.features` object which isn't strictly required but might be useful.\r\n\r\n@defunctzombie: can you make a choice between approaches based on this information, and then review the better suited candidate? Or perhaps review both, and then merge the approach where you prefer the source code? Or should we provide any other information to evaluate the relative merits of both approaches?"}}