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

stop removing session cache from memcached #1185

Merged
merged 2 commits into from Feb 2, 2017

Conversation

i110
Copy link
Contributor

@i110 i110 commented Feb 1, 2017

as discussed at #1087 (comment), we shouldn't remove tls session cache actively.

Regarding the manual, there are 3 triggers the remove callback gets called:

The remove_session_cb() is called, whenever the SSL engine removes a session from the internal cache. This happens

  • when the session is removed because it is expired
  • or when a connection was not shutdown cleanly.
  • It also happens for all sessions in the internal session cache when SSL_CTX_free is called.

For the first one, currently we're using memcached (and redis in near future) which have their own expiration feature, so there's no need to remove explicitly.
For the second, we would not want to remove a cache entry when a connection is abruptly shut down, which is often the case with the browsers.
For the last, the session cache should also still be alive.

For above reasons, I believe that we should do nothing when the remove events get fired

@kazuho
Copy link
Member

kazuho commented Feb 1, 2017

Thank you for the PR.

How about removing on_async_resumption_remove as a whole (at the same time stop calling SSL_CTX_sess_set_remove_cb)?

I don't see any need to retain the code.

@i110
Copy link
Contributor Author

i110 commented Feb 1, 2017

@kazuho Thank you for your review. I removed that.

@kazuho
Copy link
Member

kazuho commented Feb 2, 2017

Thank you very much! Less code = win.

@kazuho kazuho merged commit fd12da0 into master Feb 2, 2017
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.

None yet

2 participants