You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@trafficserver.apache.org by shinrich <gi...@git.apache.org> on 2016/06/15 13:09:25 UTC
[GitHub] trafficserver pull request #717: TS-4507: Fixes to ensure SSN_CLOSE called a...
GitHub user shinrich opened a pull request:
https://github.com/apache/trafficserver/pull/717
TS-4507: Fixes to ensure SSN_CLOSE called after TXN_CLOSE.
Adding logic to ensure that SSN_CLOSE doesn't get called before TXN_CLOSE. This involved deferring the session destroy further. Had to add some recursion checks in HTTP2 to make sure we don't destroy the ssn object on the stack.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/shinrich/trafficserver ts-4507
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/trafficserver/pull/717.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #717
----
commit 2cd181fc0294de17ca0386a33ae45be06780d496
Author: shinrich <sh...@localhost.localdomain>
Date: 2016-06-15T12:48:49Z
TS-4507: Fixes to ensure SSN_CLOSE called after TXN_CLOSE.
----
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver pull request #717: TS-4507: Fixes to ensure SSN_CLOSE called a...
Posted by shinrich <gi...@git.apache.org>.
Github user shinrich closed the pull request at:
https://github.com/apache/trafficserver/pull/717
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver pull request #717: TS-4507: Fixes to ensure SSN_CLOSE called a...
Posted by jpeach <gi...@git.apache.org>.
Github user jpeach commented on a diff in the pull request:
https://github.com/apache/trafficserver/pull/717#discussion_r68082470
--- Diff: proxy/http/Http1ClientSession.cc ---
@@ -82,11 +82,21 @@ Http1ClientSession::Http1ClientSession()
void
Http1ClientSession::destroy()
{
+}
+
+void
+Http1ClientSession::really_destroy()
--- End diff --
Really? Can you please elaborate on why another layer of destroy is needed? It seems fairly ugly to me :-/
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver issue #717: TS-4507: Fixes to ensure SSN_CLOSE called after TX...
Posted by atsci <gi...@git.apache.org>.
Github user atsci commented on the issue:
https://github.com/apache/trafficserver/pull/717
Linux build *successful*! See https://ci.trafficserver.apache.org/job/Github-Linux/177/ for details.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver issue #717: TS-4507: Fixes to ensure SSN_CLOSE called after TX...
Posted by shinrich <gi...@git.apache.org>.
Github user shinrich commented on the issue:
https://github.com/apache/trafficserver/pull/717
Yes, this is snakey logic. I have a couple more iterations after working with various teams internally. I'll be putting up a new PR for that (really_destroy goes away), so I'm closing this PR for now. It will at a minimum have a healthy comment discussion on the flow.
As far as testing goes, I will need some help with that. Ultimately, some of that will need to be shaken out through load testing with appropriate plugins. We have an intern writing up some functional test plugin archetypes to help with that.
But I agree we should be able run through some basic cases in an integration testing framework. I'll write up a list of test case requirements and either work with @dragon512 to get this exercised in next gen TSQA or work with someone else to get this going in current TSQA.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver pull request #717: TS-4507: Fixes to ensure SSN_CLOSE called a...
Posted by jpeach <gi...@git.apache.org>.
Github user jpeach commented on a diff in the pull request:
https://github.com/apache/trafficserver/pull/717#discussion_r68083417
--- Diff: proxy/ProxyClientTransaction.h ---
@@ -174,10 +174,10 @@ class ProxyClientTransaction : public VConnection
return true;
}
+ virtual void destroy();
virtual void
- destroy()
+ transaction_done()
--- End diff --
Consider a more explicit tracking of the transaction state, even a simple IDLE, ACTIVE, DONE would probably make things a lot more explicit.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver pull request #717: TS-4507: Fixes to ensure SSN_CLOSE called a...
Posted by jpeach <gi...@git.apache.org>.
Github user jpeach commented on a diff in the pull request:
https://github.com/apache/trafficserver/pull/717#discussion_r68081721
--- Diff: proxy/http/Http1ClientSession.h ---
@@ -181,7 +189,12 @@ class Http1ClientSession : public ProxyClientSession
MIOBuffer *read_buffer;
IOBufferReader *sm_reader;
- C_Read_State read_state;
+
+ /*
+ * Volatile should not be necessary, but there appears to be a bug in the 4.9 rhel gcc
+ * compiler that was using an old version of read_state to make decisions in really_destroy
+ */
+ volatile C_Read_State read_state;
--- End diff --
If there are threaded dependencies on this, then we should force an atomic read or a memory acquire/release. I think the volatile is too subtle to be depended on.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver issue #717: TS-4507: Fixes to ensure SSN_CLOSE called after TX...
Posted by atsci <gi...@git.apache.org>.
Github user atsci commented on the issue:
https://github.com/apache/trafficserver/pull/717
FreeBSD build *successful*! See https://ci.trafficserver.apache.org/job/Github-FreeBSD/285/ for details.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver issue #717: TS-4507: Fixes to ensure SSN_CLOSE called after TX...
Posted by jpeach <gi...@git.apache.org>.
Github user jpeach commented on the issue:
https://github.com/apache/trafficserver/pull/717
@shinrich I'd really like to see some exhaustive code comments about how transaction destruction is supposed to work and how to operate this machinery. Just looking at the diff it really looks like black magic.
I'm concerned about the maintainability of ``destroy()``, ``do_destroy()`` and ``really_destroy()``; maybe if you can explain the context we can come up with some cleaner way to express the intent?
Of course, the testing question :) Is there a way to test this either in regression tests or in tsqa (or its successor)?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---