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/05/26 02:23:34 UTC
[GitHub] trafficserver pull request: TS-4661: SSL Client Connections not cl...
GitHub user shinrich opened a pull request:
https://github.com/apache/trafficserver/pull/669
TS-4661: SSL Client Connections not closed.
Ultimately the issue was that SSL connections that stall in the ProtocolTrampoline never have the inactivity timeout cleanup. The problem was introduced in 6.0.0 due to an unfortunately interaction between the addition of the ssl_handshake_timeout_in and inactivity_timeout mechanism.
The problem occurs when ssl_handshake_timeout_in is set to 0, which is the scenario that @bcall and I were testing. This causes vc->set_inactivity_timeout(0) to be called. This sets vc->inactivity_timeout_in to 0 and vc->next_inactivity_timeout_at to current time.
Looking at UnixNetVConnection::mainEvent, the inactivity timeout event is not propagate if inactivity_timeout_in is 0 even if next_inactivity_timeout_at is non-zero and less than the current time.
Looking at check_inactivity, if next_inactivity_timeout_at is 0, it will call vc->set_inactivity_timeout with the default_inactivity_timeout.
But since next_inactivity_timeout_at is not 0, the default is never set and inactivity_timeout_in is never set to non-zero, so the inactivity_timeout signal is never propagated and thus the connection is never closed.
I adjusted set_inactivity_timeout to not set the next_inactivity_timeout_at if the argument is 0. This fix has been tested in production against the 6.2 code, and the client connections all close after the box is removed from traffic.
This patch also includes a fix to add the Http1ClientSessions to the appropriate _queues. That fix will be needed eventually, but wasn't essential for this particular scenario.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/shinrich/trafficserver ts-4461
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/trafficserver/pull/669.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 #669
----
commit a559295f4c36677e1f8044ac259591ce5bb48830
Author: Susan Hinrichs <sh...@ieee.org>
Date: 2016-05-26T02:09:48Z
TS-4661: SSL Client Connections not closed.
----
---
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-4461: SSL Client Connections not cl...
Posted by zwoop <gi...@git.apache.org>.
Github user zwoop commented on the pull request:
https://github.com/apache/trafficserver/pull/669#issuecomment-221936130
Cool. Can you mark the Jira with back port to 6.1.2 as well as 6.2.0 please? I'll try to spend some time today and tomorrow to see what we should get into a potential 6.1.2 release.
---
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-4461: SSL Client Connections not cl...
Posted by shinrich <gi...@git.apache.org>.
Github user shinrich commented on the pull request:
https://github.com/apache/trafficserver/pull/669#issuecomment-222003637
Merged up with commit 456ade9ab35787d39d3414149a0ee04f44008268
---
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-4461: SSL Client Connections not cl...
Posted by bryancall <gi...@git.apache.org>.
Github user bryancall commented on a diff in the pull request:
https://github.com/apache/trafficserver/pull/669#discussion_r64785961
--- Diff: proxy/http/Http1ClientSession.cc ---
@@ -427,9 +427,12 @@ Http1ClientSession::release(ProxyClientTransaction *trans)
ka_vio = this->do_io_read(this, INT64_MAX, read_buffer);
ink_assert(slave_ka_vio != ka_vio);
- // Y!?
- // client_vc->add_to_keep_alive_lru();
- client_vc->cancel_active_timeout();
+ if (client_vc) {
+ client_vc->add_to_keep_alive_queue();
+ if (client_vc) {
--- End diff --
Why not just cancel the active timeout first and then add to the keep alive queue?
---
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-4461: SSL Client Connections not cl...
Posted by shinrich <gi...@git.apache.org>.
Github user shinrich commented on a diff in the pull request:
https://github.com/apache/trafficserver/pull/669#discussion_r64788248
--- Diff: proxy/http/Http1ClientSession.cc ---
@@ -427,9 +427,12 @@ Http1ClientSession::release(ProxyClientTransaction *trans)
ka_vio = this->do_io_read(this, INT64_MAX, read_buffer);
ink_assert(slave_ka_vio != ka_vio);
- // Y!?
- // client_vc->add_to_keep_alive_lru();
- client_vc->cancel_active_timeout();
+ if (client_vc) {
+ client_vc->add_to_keep_alive_queue();
+ if (client_vc) {
--- End diff --
Yes, that does seem easier. I'll change to cancel first. That should call no continuations so the client_vc field should not vanish on us.
---
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-4461: SSL Client Connections not cl...
Posted by SolidWallOfCode <gi...@git.apache.org>.
Github user SolidWallOfCode commented on the pull request:
https://github.com/apache/trafficserver/pull/669#issuecomment-221935663
I went through all the changes with Susan and I will +1 these changes.
---
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-4461: SSL Client Connections not cl...
Posted by zwoop <gi...@git.apache.org>.
Github user zwoop commented on the pull request:
https://github.com/apache/trafficserver/pull/669#issuecomment-221927293
@shinrich The Jira vs this PR has slightly different implications :). Is this a 6.0 issue (i.e. would 6.1.x be affected by this) or is it only in 6.2.x ? I'm contemplating making a 6.1.2 stop gap release until 6.2.0 comes out, so curious if we should back port this to 6.1.2 as well.
---
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-4461: SSL Client Connections not cl...
Posted by shinrich <gi...@git.apache.org>.
Github user shinrich closed the pull request at:
https://github.com/apache/trafficserver/pull/669
---
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-4461: SSL Client Connections not cl...
Posted by shinrich <gi...@git.apache.org>.
Github user shinrich commented on the pull request:
https://github.com/apache/trafficserver/pull/669#issuecomment-221935540
Based on my research it would be an issue for 6.0 and 6.1 as well. TS-3727 change was marked fixed for 6.0.
---
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-4461: SSL Client Connections not cl...
Posted by SolidWallOfCode <gi...@git.apache.org>.
Github user SolidWallOfCode commented on a diff in the pull request:
https://github.com/apache/trafficserver/pull/669#discussion_r64783870
--- Diff: proxy/http/Http1ClientSession.cc ---
@@ -427,9 +427,12 @@ Http1ClientSession::release(ProxyClientTransaction *trans)
ka_vio = this->do_io_read(this, INT64_MAX, read_buffer);
ink_assert(slave_ka_vio != ka_vio);
- // Y!?
- // client_vc->add_to_keep_alive_lru();
- client_vc->cancel_active_timeout();
+ if (client_vc) {
+ client_vc->add_to_keep_alive_queue();
+ if (client_vc) {
--- End diff --
Susan had to explain this to me because I'm kind of slow but I think it might be good to have a comment about why `client_vc` needs to be checked again.
---
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.
---