New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix leaks #41

Merged
merged 4 commits into from Dec 29, 2013

Conversation

Projects
None yet
3 participants
@mmalecki
Copy link
Contributor

mmalecki commented Nov 18, 2013

@indexzero can you still reproduce leaks with this branch? These are all the leaks leaks tool reported.

@indexzero

This comment has been minimized.

Copy link
Member

indexzero commented Nov 19, 2013

@mmalecki Awesome. @jcrugzz can you take a look at this when you're before you go to Philly?

@indexzero indexzero referenced this pull request Nov 19, 2013

Open

There is a memory leak #5

@jcrugzz

This comment has been minimized.

Copy link
Member

jcrugzz commented Nov 19, 2013

@indexzero yea ill check this out today.

@jcrugzz

This comment has been minimized.

Copy link
Member

jcrugzz commented Nov 20, 2013

@mmalecki Im getting a seg fault running the command found below locally. Im connecting to a standard TCP server at localhost:3001 that logs to the console. It seems to have to do with writing the start.log file. Also did you take a look at @Southern's d0d2aae commit? I'm a bit rusty on my C but this seems sane since saneopt only malloc's the opt var.

/forza -h localhost -p 3001 -- test/fixtures/listen-0.js
forza 0.2.2
connecting to 127.0.0.1:3001...
connected.
uv_write: bad file descriptor
forza(93873,0x7fff716af180) malloc: *** error for object 0x7fff5893c92b: pointer being freed was not allocated
*** set a breakpoint in malloc_error_break to debug
[1]    93873 abort      ./forza -h localhost -p 3001 -- test/fixtures/listen-0.js
@jcrugzz

This comment has been minimized.

Copy link
Member

jcrugzz commented Nov 20, 2013

The current master has the same uv_write: bad file descriptor error but does not hit this pointer being freed error

@jcrugzz

This comment has been minimized.

Copy link
Member

jcrugzz commented Nov 20, 2013

ah yes makes sense why the file descriptor error is happening. But yea we shouldnt break if there is no start-log.

@jcrugzz

This comment has been minimized.

Copy link
Member

jcrugzz commented Nov 24, 2013

yea im just retarded its fine. But @mmalecki I'm still seeing a 304 bytes leak coming from env_copy on OS X. Much better than before though.

image

Hmm I'm thinking that it might just not be smart enough to pick up on the reassignment here since it is added to the uv_process_options_t options. Because the logic for freeing it seems sane unless its some off by one error.

@jcrugzz jcrugzz merged commit 7dff516 into nodejitsu:master Dec 29, 2013

@mmalecki mmalecki deleted the mmalecki:fix-leaks branch Dec 30, 2013

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment