Skip to content

Conversation

@shinrich
Copy link
Member

@shinrich shinrich commented Jul 2, 2016

This change was motivated by dealing with ordering problems between SSN_CLOSE and TXN_CLOSE causing problems (crashes). This we addressed as follows

  • The TXN_CLOSE is kicked off in the State Machine kill_this. This is the point were we know that the TXN really is going away. This is unchanged. What this patch adds is a call to transaction_done to let the ClientTransaction know that TXN_CLOSE has completed so the calculation about whether is it time to execute SSN_CLOSE or not. As such we cannot null out ua_session early.
  • To support SSN_CLOSE accurately, we split the destroy logic into two parts: destroy and free. Destroy commits to deleting the session and it kicks off the SSN_CLOSE plugin. free performs the final resource reclamation of the session object.

In addition this PR includes the following fixes.

  • In ProxyClientSession::state_api_callout we schedule_in 10ms in the future if the plugin lock is not acquired. Saw ASAN use-after-free crashes when the Http2ClientSession is deleted but the schedule event remains and is triggered. Added a schedule_event member to track this case and cancel any outstanding schedule events on free.
  • Cleaned up handling regular events at the same time as plugin events. The original code relied on the subclasses overriding handle_api_event to handle the regular events, but the handler only handled the TIMEOUT event. Changed that to augment the subclasses' main event handler to call out to state_api_callout in the event of the plugin events.

@atsci
Copy link

atsci commented Jul 2, 2016

FreeBSD build successful! See https://ci.trafficserver.apache.org/job/Github-FreeBSD/407/ for details.

@atsci
Copy link

atsci commented Jul 2, 2016

Linux build successful! See https://ci.trafficserver.apache.org/job/Github-Linux/301/ for details.

@shinrich
Copy link
Member Author

shinrich commented Jul 3, 2016

These fixes improved the stability of our 5.3.x + TS-3612 build. I'm heading out until July 14, but I wanted to make these changes available in case anyone else was seeing stability problems in this area.

@jpeach
Copy link
Contributor

jpeach commented Jul 5, 2016

Is is possible to break the addditional fixes out into separate tickets and PRs?

@shinrich
Copy link
Member Author

It would be possible to break out the schedule_event clean up and the state_api_callout into smaller patches. The rest of it really needs to be together. I need to move onto another project by Monday. I'll see what I can get done this afternoon. Otherwise these fixes will need to wait a while.

@shinrich
Copy link
Member Author

Pushed new version of the branch that does not include the schedule_event and state_api_callout fixes. Filed TS-4663 and TS-4664 to track those issues. Will try to reproduce those fixes on new branches.

@atsci
Copy link

atsci commented Jul 15, 2016

FreeBSD build failed! See https://ci.trafficserver.apache.org/job/Github-FreeBSD/432/ for details.

@atsci
Copy link

atsci commented Jul 15, 2016

Linux build successful! See https://ci.trafficserver.apache.org/job/Github-Linux/325/ for details.

@SolidWallOfCode
Copy link
Member

I've reviewed the changes and I don't think they can really be split up. Part of the reason is similar changes in both the HTTP/1 and HTTP/2 classes, which makes the change appear bigger than it is. I'm +1 on committing this, especially since we've been successfully testing them in production inside Yahoo!.

@shinrich shinrich merged commit e0664e4 into apache:master Aug 4, 2016
bneradt added a commit to bneradt/trafficserver that referenced this pull request Jul 12, 2024
This cherry-picks two PRs from Vinith to 10.0.x from 9.1.x:

* apache#779
* apache#784
masaori335 pushed a commit to masaori335/trafficserver that referenced this pull request May 29, 2025
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.

5 participants