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

Set nullptr to ua_session after it is destoryed #1455

Merged
merged 1 commit into from
Feb 18, 2017

Conversation

masaori335
Copy link
Contributor

@masaori335 masaori335 commented Feb 16, 2017

Issue:
Crash by EXC_BAD_ACCESS in Http2ConnectionState::release_stream() under heavy load

Cause:
While total_connections_in is larger than max_connections_per_thread_in (in NetHandler::manage_keep_alive_queue()),
Http2ConnectionState::release_stream() is called recurcively from add_to_keep_alive_queue().
At the bottom of recursion, ua_session is destroyed and Http2ConnectionState::release_stream() access to it.

Fix:

  1. Set nullptr to ua_session after it is destoryed
  2. Swap calls of add_to_keep_alive_queue() and cancel_active_timeout() for ua_session nullptr check
  3. Check m_active of ua_session to reduce recursion

@masaori335 masaori335 added this to the 7.1.0 milestone Feb 16, 2017
@masaori335 masaori335 self-assigned this Feb 16, 2017
@masaori335
Copy link
Contributor Author

We need to backport this to 7.1.x ( I'll send another PR after we verified this works well)

@atsci
Copy link

atsci commented Feb 16, 2017

FreeBSD build successful! See https://ci.trafficserver.apache.org/job/freebsd-github/1559/ for details.

@atsci
Copy link

atsci commented Feb 16, 2017

Linux build successful! See https://ci.trafficserver.apache.org/job/linux-github/1454/ for details.

@atsci
Copy link

atsci commented Feb 16, 2017

clang-analyzer build successful! See https://ci.trafficserver.apache.org/job/clang-analyzer-github/124/ for details.

@atsci
Copy link

atsci commented Feb 16, 2017

FreeBSD build successful! See https://ci.trafficserver.apache.org/job/freebsd-github/1561/ for details.

@atsci
Copy link

atsci commented Feb 16, 2017

Linux build successful! See https://ci.trafficserver.apache.org/job/linux-github/1455/ for details.

Issue:
Crash by EXC_BAD_ACCESS in Http2ConnectionState::release_stream() under heavy load

Cause:
While total_connections_in is larger than max_connections_per_thread_in (in NetHandler::manage_keep_alive_queue()),
Http2ConnectionState::release_stream() is called recurcively from add_to_keep_alive_queue().
At the bottom of recursion, ua_session is destroyed and Http2ConnectionState::release_stream() access to it.

Fix:
1. Set nullptr to ua_session after it is destoryed
2. Swap calls of add_to_keep_alive_queue() and cancel_active_timeout() for ua_session nullptr check
3. Check m_active of ua_session to reduce recursion
@atsci
Copy link

atsci commented Feb 16, 2017

clang-analyzer build successful! See https://ci.trafficserver.apache.org/job/clang-analyzer-github/126/ for details.

@atsci
Copy link

atsci commented Feb 16, 2017

FreeBSD build successful! See https://ci.trafficserver.apache.org/job/freebsd-github/1562/ for details.

@atsci
Copy link

atsci commented Feb 16, 2017

Linux build successful! See https://ci.trafficserver.apache.org/job/linux-github/1456/ for details.

@atsci
Copy link

atsci commented Feb 16, 2017

clang-analyzer build successful! See https://ci.trafficserver.apache.org/job/clang-analyzer-github/127/ for details.

Copy link
Contributor

@zwoop zwoop left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Been running with this on Docs and on a small number of prod boxes without problems. I'm comfortable landing this.

@zwoop zwoop merged commit b952716 into apache:master Feb 18, 2017
@zwoop
Copy link
Contributor

zwoop commented Feb 18, 2017

I cherry-picked this to 7.1.x, so no need to make an additional PR.

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

Successfully merging this pull request may close these issues.

None yet

3 participants