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

Allow cleanup of redis sockets to prevent event core leaking. #2372

Merged
merged 3 commits into from Oct 17, 2019

Conversation

schveiguy
Copy link
Contributor

See issue #2245. This allows user code to clean up a RedisClient or RedisSessionStore before exiting the program to prevent messages of leaked eventcore drivers. Not attached to the function name, so if that needs to change, let me know.

@Geod24
Copy link
Contributor

Geod24 commented Sep 30, 2019

Any way this could be tested ?

@schveiguy
Copy link
Contributor Author

I'm trying to think of how to test this without having an actual Redis server...

Indeed, there's only 2 unittests in this file, and they both do not work with the actual Redis server.

I'm open to ideas, but I don't know if it can be done.

@s-ludwig
Copy link
Member

s-ludwig commented Oct 9, 2019

I'm trying to think of how to test this without having an actual Redis server...

Indeed, there's only 2 unittests in this file, and they both do not work with the actual Redis server.

There is a Redis integration test in the "tests" directory where this could be added. The only way to verify this that I can see is to manually count all open file descriptors/SOCKET handles, before and after. I'd not make this mandatory, but it would be nice to add a simple call in the existing test, so that ideally the current warnings go away (CI output).

@schveiguy
Copy link
Contributor Author

There is a Redis integration test in the "tests" directory where this could be added.

OK, will do.

@schveiguy
Copy link
Contributor Author

Updated

connections in redis tests to at least exercise the code path.
@schveiguy
Copy link
Contributor Author

Ugh, I keep screwing up my redis test. I couldn't run the test locally because I don't have a redis server that I want to muck with. In any case, I had to force push again. If it turns green, I'll ping you to reapply the auto merge.

@schveiguy
Copy link
Contributor Author

OK, ping @s-ludwig. The failure seems unrelated to the patch.

@schveiguy
Copy link
Contributor Author

ping @s-ludwig again.

@s-ludwig
Copy link
Member

Sorry, missed the first time! Restarted and toggling auto-merge.

@dlang-bot dlang-bot merged commit f26dd28 into vibe-d:master Oct 17, 2019
@schveiguy schveiguy deleted the rediscleanup branch October 17, 2019 15:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants