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/04/14 17:41:30 UTC
[GitHub] trafficserver pull request: Ts 3612 - Restructuring client session...
GitHub user shinrich opened a pull request:
https://github.com/apache/trafficserver/pull/570
Ts 3612 - Restructuring client session and transaction processing
This has been running on a 5.3 base in our production in a few machines for about a month. We are starting to roll it out more widely. I've also run it in our production for a few days on a 6.1 base. It had some performance issues, but the base 6.1 had the same performance issues which I've filed bugs on. Also ran a short while with these changes on master. Again some problems, but the same problems were on the base code in our environment as well.
These changes build upon James' abstraction of the ProxyClientSession. We introduce a ProxyClientTransaction. HttpSM works with the ProxyClientTransaction instead of the HttpClientSession. Http1 and Http2 provide subclassed instances of the ProxyClientTransaction. This takes FetchSM out of the picture for Http2. https://www.dropbox.com/s/j87lph2z66vx05t/ProxyClientSessionRe-architectProposal.pdf?dl=0 contains the design document. We did not do the Spdy aspects of the design.
SPDY is unchanged. It still uses FetchSM and seems to work as well has it has been. There are minimal changes in the Spdy code.
The bulk of the code changes are directly attributed to the Session/Transaction separation. In addition, there are a few other changes.
* When moving to 6.1 I had memory freeing issues with some of the changes for SSL buffering introduced in TS-3714. Since these changes were addressing issues in 5.0 that were fixed a different way in 5.2, I pulled out the extra buffering as unnecessary.
* I rolled back the changes for TS-4321 which were merged up yesterday. Sorry, I should have jumped on that sooner. Once FetchSM is out of the picture, we should discuss the best way to reduce response header buffering.
* I made some changes in solve InkConInternal deletes (or lack of deletes) that occurred due to changes in the event_counts with the fetchSM removal.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/shinrich/trafficserver ts-3612
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/trafficserver/pull/570.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 #570
----
commit f4999c35cf4e877b0f36e31a739b1297f3a54bee
Author: Susan Hinrichs <sh...@draggingnagging.corp.ne1.yahoo.com>
Date: 2016-04-13T19:57:39Z
TS-3612: Restructure client session and transaction processing.
commit 748aa23b1c2fd2c99d125e76ecbe97d70cfbcdcf
Author: shinrich <sh...@yahoo-inc.com>
Date: 2016-04-14T14:50:39Z
Clang format
commit 1f0b35f13b128c486b0efa71106f27d893fe82ba
Author: shinrich <sh...@yahoo-inc.com>
Date: 2016-04-14T15:24:24Z
Revert "TS-4321: Keep response headers in FetchSM as they are (#551)"
This reverts commit 60d07be8b199cc843c5e220ac0f6ed0545040422.
Conflicts:
proxy/http2/Http2ConnectionState.cc
Taking this out for now to put in TS-3612. Once FetchSM is out of the way, we can talk about the
best way to optimize this case.
----
---
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: Ts 3612 - Restructuring client session...
Posted by calavera <gi...@git.apache.org>.
Github user calavera commented on the pull request:
https://github.com/apache/trafficserver/pull/570#issuecomment-218931181
@asfgit @jpeach I just want to leave a note here because we found an issue with this commit. Under certain circumstances ATS segfaults due to this changes. I haven't been able to find the real issue yet but it might be prudent to flag this commit before 7.0 ships. I'll open an issue as soon as I have more information.
---
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: Ts 3612 - Restructuring client session...
Posted by jpeach <gi...@git.apache.org>.
Github user jpeach commented on the pull request:
https://github.com/apache/trafficserver/pull/570#issuecomment-219107804
@calavera or crash logs :)
---
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: Ts 3612 - Restructuring client session...
Posted by zwoop <gi...@git.apache.org>.
Github user zwoop commented on the pull request:
https://github.com/apache/trafficserver/pull/570#issuecomment-218931643
We should flag this for 6.2.0. Can you file a Jira please? @PSUdaemon
---
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: Ts 3612 - Restructuring client session...
Posted by maskit <gi...@git.apache.org>.
Github user maskit commented on a diff in the pull request:
https://github.com/apache/trafficserver/pull/570#discussion_r59965426
--- Diff: proxy/ProxyClientSession.h ---
@@ -101,6 +106,75 @@ class ProxyClientSession : public VConnection
void do_api_callout(TSHttpHookID id);
static int64_t next_connection_id();
+ virtual int get_transact_count() const = 0;
+
+ // Override if your session protocol allows this
+ virtual bool
+ is_transparent_passthrough_allowed()
+ {
+ return false;
+ }
+
+ // Override if your session protocol cares
+ virtual void
+ set_half_close_flag(bool flag)
+ {
+ }
+ virtual bool
+ get_half_close_flag()
--- End diff --
This one should be const?
```
In file included from HttpSessionAccept.cc:26:
./Http1ClientSession.h:83:3: error: 'Http1ClientSession::get_half_close_flag' hides overloaded virtual function [-Werror,-Woverloaded-virtual]
get_half_close_flag() const
^
../../proxy/ProxyClientSession.h:124:3: note: hidden overloaded virtual function 'ProxyClientSession::get_half_close_flag' declared here: different qualifiers (none vs const)
get_half_close_flag()
^
```
---
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: Ts 3612 - Restructuring client session...
Posted by maskit <gi...@git.apache.org>.
Github user maskit commented on a diff in the pull request:
https://github.com/apache/trafficserver/pull/570#discussion_r59969775
--- Diff: proxy/ProxyClientSession.h ---
@@ -101,6 +106,75 @@ class ProxyClientSession : public VConnection
void do_api_callout(TSHttpHookID id);
static int64_t next_connection_id();
+ virtual int get_transact_count() const = 0;
+
+ // Override if your session protocol allows this
+ virtual bool
+ is_transparent_passthrough_allowed()
+ {
+ return false;
+ }
+
+ // Override if your session protocol cares
+ virtual void
+ set_half_close_flag(bool flag)
+ {
+ }
+ virtual bool
+ get_half_close_flag()
--- End diff --
I'm using clang. That's why I use it.
---
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: Ts 3612 - Restructuring client session...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/trafficserver/pull/570
---
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: Ts 3612 - Restructuring client session...
Posted by shinrich <gi...@git.apache.org>.
Github user shinrich commented on a diff in the pull request:
https://github.com/apache/trafficserver/pull/570#discussion_r59967433
--- Diff: proxy/ProxyClientSession.h ---
@@ -101,6 +106,75 @@ class ProxyClientSession : public VConnection
void do_api_callout(TSHttpHookID id);
static int64_t next_connection_id();
+ virtual int get_transact_count() const = 0;
+
+ // Override if your session protocol allows this
+ virtual bool
+ is_transparent_passthrough_allowed()
+ {
+ return false;
+ }
+
+ // Override if your session protocol cares
+ virtual void
+ set_half_close_flag(bool flag)
+ {
+ }
+ virtual bool
+ get_half_close_flag()
--- End diff --
Yes, it should. Fixing. What compiler are you using? It is giving better warnings.
---
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: Ts 3612 - Restructuring client session...
Posted by calavera <gi...@git.apache.org>.
Github user calavera commented on the pull request:
https://github.com/apache/trafficserver/pull/570#issuecomment-219174356
@jpeach, @shinrich I opened an issue:
https://issues.apache.org/jira/browse/TS-4444
---
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: Ts 3612 - Restructuring client session...
Posted by jpeach <gi...@git.apache.org>.
Github user jpeach commented on the pull request:
https://github.com/apache/trafficserver/pull/570#issuecomment-210011350
Please squash the clang-format commit. Can you elaborate on why this reverts TS-4321?
---
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: Ts 3612 - Restructuring client session...
Posted by shinrich <gi...@git.apache.org>.
Github user shinrich commented on the pull request:
https://github.com/apache/trafficserver/pull/570#issuecomment-219107663
@calavera could you share a stack trace?
---
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: Ts 3612 - Restructuring client session...
Posted by maskit <gi...@git.apache.org>.
Github user maskit commented on the pull request:
https://github.com/apache/trafficserver/pull/570#issuecomment-210199731
Reverting TS-4321 sounds reasonable if we can modify ``stream->response_header`` without any side effects.
The response headers in FetchSM must not be modified because FetchSM uses them internally to check if the response body is chunked. It seems ``Http2Stream::response_initialize_data_handling`` does the similar thing.
So all checks depend on response headers must be done before we modify headers here if we don't make the copy. If we cannot assure that, we still need the fix for TS-4321.
Moreover, generating H2 headers from headers in Http2Stream sounds odd.
---
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: Ts 3612 - Restructuring client session...
Posted by biilmann <gi...@git.apache.org>.
Github user biilmann commented on the pull request:
https://github.com/apache/trafficserver/pull/570#issuecomment-220918532
This pull request also seems to break one of our plugins that does HTTP requests in a background worker thread via `TSFetchUrl` (used indirectly through the `AsyncHttpFetch` C++ API).
The HTTP request will hang at this check for `the right thread` and never progress:
https://github.com/apache/trafficserver/pull/570/files#diff-40fc32ab3504372e0eb74a54108d1b6fR2404
Removing these checks for the right thread leads to a segfault.
---
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: Ts 3612 - Restructuring client session...
Posted by maskit <gi...@git.apache.org>.
Github user maskit commented on a diff in the pull request:
https://github.com/apache/trafficserver/pull/570#discussion_r59965264
--- Diff: proxy/ProxyClientSession.h ---
@@ -25,14 +25,19 @@
#define __PROXY_CLIENT_SESSION_H__
#include "ts/ink_platform.h"
+#include "ts/ink_resolver.h"
#include "P_Net.h"
#include "InkAPIInternal.h"
+#include "http/HttpServerSession.h"
// Emit a debug message conditional on whether this particular client session
// has debugging enabled. This should only be called from within a client session
// member function.
#define DebugSsn(ssn, tag, ...) DebugSpecific((ssn)->debug(), tag, __VA_ARGS__)
+class ProxyClientTransaction;
+class AclRecord;
--- End diff --
I got this error.
```
In file included from ./Http1ClientSession.h:41:
../../proxy/ProxyClientSession.h:39:1: error: class 'AclRecord' was previously declared as a struct [-Werror,-Wmismatched-tags]
class AclRecord;
^
../../proxy/IPAllow.h:56:8: note: previous use is here
struct AclRecord {
^
../../proxy/ProxyClientSession.h:39:1: note: did you mean struct here?
class AclRecord;
^~~~~
struct
```
---
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: Ts 3612 - Restructuring client session...
Posted by shinrich <gi...@git.apache.org>.
Github user shinrich commented on the pull request:
https://github.com/apache/trafficserver/pull/570#issuecomment-210037794
I figured there would be some more updates via the review process so I delayed further squashes. Will definitely squash before the final commit.
Since we aren't using FetchSM with this re-arrangement, it wasn't clear that we needed to do the function rearrangements and extra header copies and allocations. Actually read the bug report more closely just now rather than just looking at the code changes. It isn't clear to me that this is an issue without FetchSM, but I'll go head and apply the function changes here, and repush.
---
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.
---