al6x / synchronize

Write asynchronous code as if it's synchronous
http://alexeypetrushin.github.com/synchronize
316 stars 57 forks source link

enrich errors call stack #39

Closed mihaislobozeanu closed 8 years ago

mihaislobozeanu commented 8 years ago

Hi,

I've made some changes to get a more relevant call stack on error.

Thanks.

mihaislobozeanu commented 8 years ago

I have the latest mocha version and locally this error is not raised.

d3m3vilurr commented 8 years ago

Ok. I checked this pr (our company also want this pr ;) ) mocha was all passed on local machine, but travis still failed. so, patched pr code, like this https://github.com/d3m3vilurr/synchronize/commit/0c04d258dda0 after patch, test all passed.

@mihaislobozeanu can you merge this patch?

d3m3vilurr commented 8 years ago

ah...wait my patch really cause leak problem. :'(

d3m3vilurr commented 8 years ago

Sorry messages. This is final fix https://github.com/d3m3vilurr/synchronize/commit/e132c9918 :)

mihaislobozeanu commented 8 years ago

Done, thanks. One question. Why it was a memory leak?

d3m3vilurr commented 8 years ago
  1. original pr not have leak. but basically, this variable is global variable. (not have let or var keyword). so, this patch will give wrong result.
  2. my first patch have real leak. node process cannot detect in fiber object is same Fiber.current, it cause c++ module(node-fibers implements, Fiber.current will return GetCurrent() result. so it will have not same address). so we can't pass outside variable to fiber.run closure, it will cause that variables not mark GC target(yes, it's insane.).
  3. but, logically fiber variable is same Fiber.current. so we can use this context to pass variable.

32 help about this problem.

al6x commented 8 years ago

Hi guys, sorry for delay, didn't had much free time recently.

@d3m3vilurr I added you to contributors, if you wish please feel free to merge this PR. I also bumped the version to 2 for the new development. (v1 will be kept frozen, only for backward compatible fixes).

d3m3vilurr commented 8 years ago

thx invite :) btw i think this pr is not enough. so i will add more patch at tomorrow.

d3m3vilurr commented 8 years ago

close this pr. we will handle #49

d3m3vilurr commented 8 years ago

merged a42337bd6d