Skip to content
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

release ref on failing connectTCP #116

Merged
merged 2 commits into from
Jan 19, 2019
Merged

Conversation

WebFreak001
Copy link
Contributor

@WebFreak001 WebFreak001 commented Jan 8, 2019

fix #115

Unsure how this affects windows, but if the auto tests are also run on windows it should find any issue there. please test manually

status is refused in this case and it's not handled by the existing cases cancelling a connection, so the reference to the socket still exists and needs to be released by the caller here.

@s-ludwig
Copy link
Member

It looks like this is an issue in eventcore. All other error codes result in the socket being closed, but ConnectionStatus.refused keeps it open (direct reason for the difference is that all other codes are generated immediately, while refused is an event loop event). I'll add a call to releaseRef right after the user callback has been invoked, so that the socket handle is still valid during the callback.

@s-ludwig
Copy link
Member

Upon taking a closed look, the behavior in eventcore makes sense, because otherwise the handle already returned by connectStream would be silently invalidated. So fixing it here is the right thing to do after all.

It would be nice to get the test case to actually fail. The only way I see right now is to count the number of open files at the start and at the end of the test case (e.g. using getrlimit). I'd look into the Windows version of this.

@WebFreak001
Copy link
Contributor Author

Can't we make the "get leaked handles" message as a public function in eventcore?

@s-ludwig
Copy link
Member

Doing that properly would be a bit more involved, because the leaked handles may be scattered over multiple threads/drivers (in TLS-memory), so the first step would be a global registry for driver instances and thread-safe way to query leaked handles of foreign threads. The getrlimit variant would be a oneliner using iota/count on the other hand.

@s-ludwig s-ludwig merged commit 568cdb1 into vibe-d:master Jan 19, 2019
@s-ludwig
Copy link
Member

I'll tackle the test additions in a separate branch.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

connectTCP failure leaking eventcore driver handles
2 participants