You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@trafficserver.apache.org by oknet <gi...@git.apache.org> on 2016/09/19 06:14:49 UTC
[GitHub] trafficserver pull request #1033: TS-4879: Checking con.fd == NO_FD while we...
GitHub user oknet opened a pull request:
https://github.com/apache/trafficserver/pull/1033
TS-4879: Checking con.fd == NO_FD while we get return from check_emergency_throttle()
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/oknet/trafficserver TS-4879
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/trafficserver/pull/1033.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 #1033
----
commit dfed0e6404e2d0cec78dde18130dd351816ac33f
Author: Oknet Xu <xu...@skyguard.com.cn>
Date: 2016-09-19T06:11:56Z
TS-4879: Checking con.fd == NO_FD while we get return from check_emergency_throttle()
----
---
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 #1033: TS-4879: Checking con.fd == NO_FD while we get re...
Posted by oknet <gi...@git.apache.org>.
Github user oknet commented on the issue:
https://github.com/apache/trafficserver/pull/1033
@jpeach , There is a similar bug in NetAccept::do_blocking_accept() which belongs to ACCEPT side instead of CONNECT side. Should I combine them together in one commit or 2 commits respectively ?
---
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 #1033: TS-4879: Checking con.fd == NO_FD while we...
Posted by jpeach <gi...@git.apache.org>.
Github user jpeach commented on a diff in the pull request:
https://github.com/apache/trafficserver/pull/1033#discussion_r79421321
--- Diff: iocore/net/UnixNetVConnection.cc ---
@@ -1273,14 +1273,21 @@ UnixNetVConnection::connectUp(EThread *t, int fd)
con.is_bound = true;
}
+ if (check_emergency_throttle(con)) {
+ // `con' could be closed if there is hyper emergency
+ if (con.fd == NO_FD) {
+ NET_SUM_GLOBAL_DYN_STAT(net_connections_currently_open_stat, -1);
+ // set errno force to EMFILE (reached limit for open file descriptors)
+ res = errno = EMFILE;
--- End diff --
``connectUP`` returns ``CONNECT_SUCCESS``, ``CONNECT_FAILURE``, or ``-errno``.
---
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 #1033: TS-4879: Checking con.fd == NO_FD while we get re...
Posted by atsci <gi...@git.apache.org>.
Github user atsci commented on the issue:
https://github.com/apache/trafficserver/pull/1033
FreeBSD build *successful*! See https://ci.trafficserver.apache.org/job/Github-FreeBSD/831/ 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 #1033: TS-4879: Checking con.fd == NO_FD while we get re...
Posted by atsci <gi...@git.apache.org>.
Github user atsci commented on the issue:
https://github.com/apache/trafficserver/pull/1033
FreeBSD build *successful*! See https://ci.trafficserver.apache.org/job/Github-FreeBSD/834/ 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 #1033: TS-4879: Checking con.fd == NO_FD while we get re...
Posted by atsci <gi...@git.apache.org>.
Github user atsci commented on the issue:
https://github.com/apache/trafficserver/pull/1033
Linux build *failed*! See https://ci.trafficserver.apache.org/job/Github-Linux/727/ 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 pull request #1033: TS-4879: Checking con.fd == NO_FD while we...
Posted by jpeach <gi...@git.apache.org>.
Github user jpeach commented on a diff in the pull request:
https://github.com/apache/trafficserver/pull/1033#discussion_r79422164
--- Diff: iocore/net/UnixNetVConnection.cc ---
@@ -1273,14 +1273,21 @@ UnixNetVConnection::connectUp(EThread *t, int fd)
con.is_bound = true;
}
+ if (check_emergency_throttle(con)) {
+ // `con' could be closed if there is hyper emergency
+ if (con.fd == NO_FD) {
+ NET_SUM_GLOBAL_DYN_STAT(net_connections_currently_open_stat, -1);
--- End diff --
Please comment here that we need to decrement the stat because ``close_UnixNetVConnection`` only decrements with a valid connection descriptor.
---
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 #1033: TS-4879: Checking con.fd == NO_FD while we get re...
Posted by atsci <gi...@git.apache.org>.
Github user atsci commented on the issue:
https://github.com/apache/trafficserver/pull/1033
FreeBSD build *successful*! See https://ci.trafficserver.apache.org/job/Github-FreeBSD/851/ 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 pull request #1033: TS-4879: Checking con.fd == NO_FD while we...
Posted by oknet <gi...@git.apache.org>.
Github user oknet commented on a diff in the pull request:
https://github.com/apache/trafficserver/pull/1033#discussion_r79530993
--- Diff: iocore/net/UnixNetVConnection.cc ---
@@ -1273,14 +1273,21 @@ UnixNetVConnection::connectUp(EThread *t, int fd)
con.is_bound = true;
}
+ if (check_emergency_throttle(con)) {
+ // `con' could be closed if there is hyper emergency
+ if (con.fd == NO_FD) {
+ NET_SUM_GLOBAL_DYN_STAT(net_connections_currently_open_stat, -1);
+ // set errno force to EMFILE (reached limit for open file descriptors)
+ res = errno = EMFILE;
+ goto fail;
+ }
+ }
+
// Must connect after EventIO::Start() to avoid a race condition
// when edge triggering is used.
if (ep.start(get_PollDescriptor(t), this, EVENTIO_READ | EVENTIO_WRITE) < 0) {
- lerrno = errno;
Debug("iocore_net", "connectUp : Failed to add to epoll list");
- action_.continuation->handleEvent(NET_EVENT_OPEN_FAILED, (void *)0); // 0 == res
- free(t);
- return CONNECT_FAILURE;
+ goto fail;
--- End diff --
The res declare with init value 0. I will update the res to return value of ep.start().
---
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 #1033: TS-4879: Checking con.fd == NO_FD while we get re...
Posted by atsci <gi...@git.apache.org>.
Github user atsci commented on the issue:
https://github.com/apache/trafficserver/pull/1033
Linux build *successful*! See https://ci.trafficserver.apache.org/job/Github-Linux/730/ 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 #1033: TS-4879: Checking con.fd == NO_FD while we get re...
Posted by atsci <gi...@git.apache.org>.
Github user atsci commented on the issue:
https://github.com/apache/trafficserver/pull/1033
Linux build *failed*! See https://ci.trafficserver.apache.org/job/Github-Linux/732/ 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 #1033: TS-4879: Checking con.fd == NO_FD while we get re...
Posted by oknet <gi...@git.apache.org>.
Github user oknet commented on the issue:
https://github.com/apache/trafficserver/pull/1033
updated per @jpeach 's comments
---
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 #1033: TS-4879: Checking con.fd == NO_FD while we get re...
Posted by atsci <gi...@git.apache.org>.
Github user atsci commented on the issue:
https://github.com/apache/trafficserver/pull/1033
FreeBSD build *successful*! See https://ci.trafficserver.apache.org/job/Github-FreeBSD/836/ 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 #1033: TS-4879: Checking con.fd == NO_FD while we get re...
Posted by atsci <gi...@git.apache.org>.
Github user atsci commented on the issue:
https://github.com/apache/trafficserver/pull/1033
Linux build *failed*! See https://ci.trafficserver.apache.org/job/Github-Linux/746/ 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 #1033: TS-4879: Checking con.fd == NO_FD while we get re...
Posted by atsci <gi...@git.apache.org>.
Github user atsci commented on the issue:
https://github.com/apache/trafficserver/pull/1033
FreeBSD build *failed*! See https://ci.trafficserver.apache.org/job/Github-FreeBSD/833/ 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 #1033: TS-4879: Checking con.fd == NO_FD while we get re...
Posted by oknet <gi...@git.apache.org>.
Github user oknet commented on the issue:
https://github.com/apache/trafficserver/pull/1033
@jpeach updated upon your comments. thanks.
---
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 #1033: TS-4879: Checking con.fd == NO_FD while we...
Posted by oknet <gi...@git.apache.org>.
Github user oknet commented on a diff in the pull request:
https://github.com/apache/trafficserver/pull/1033#discussion_r79531088
--- Diff: iocore/net/UnixNetVConnection.cc ---
@@ -1273,14 +1273,21 @@ UnixNetVConnection::connectUp(EThread *t, int fd)
con.is_bound = true;
}
+ if (check_emergency_throttle(con)) {
+ // `con' could be closed if there is hyper emergency
+ if (con.fd == NO_FD) {
+ NET_SUM_GLOBAL_DYN_STAT(net_connections_currently_open_stat, -1);
+ // set errno force to EMFILE (reached limit for open file descriptors)
+ res = errno = EMFILE;
--- End diff --
set res to -errno.
---
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 #1033: TS-4879: Checking con.fd == NO_FD while we get re...
Posted by atsci <gi...@git.apache.org>.
Github user atsci commented on the issue:
https://github.com/apache/trafficserver/pull/1033
Linux build *failed*! See https://ci.trafficserver.apache.org/job/Github-Linux/729/ 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 pull request #1033: TS-4879: Checking con.fd == NO_FD while we...
Posted by oknet <gi...@git.apache.org>.
Github user oknet commented on a diff in the pull request:
https://github.com/apache/trafficserver/pull/1033#discussion_r79530800
--- Diff: iocore/net/UnixNetVConnection.cc ---
@@ -1273,14 +1273,21 @@ UnixNetVConnection::connectUp(EThread *t, int fd)
con.is_bound = true;
}
+ if (check_emergency_throttle(con)) {
--- End diff --
changed the return type to bool for emergency_throttle, check_net_throttle and check_emergency_throttle
---
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 #1033: TS-4879: Checking con.fd == NO_FD while we...
Posted by jpeach <gi...@git.apache.org>.
Github user jpeach commented on a diff in the pull request:
https://github.com/apache/trafficserver/pull/1033#discussion_r79422378
--- Diff: iocore/net/UnixNetVConnection.cc ---
@@ -1273,14 +1273,21 @@ UnixNetVConnection::connectUp(EThread *t, int fd)
con.is_bound = true;
}
+ if (check_emergency_throttle(con)) {
--- End diff --
Since you are touching this code, could you please change the return type of ``check_emergency_throttle`` to ``bool``?
---
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 #1033: TS-4879: Checking con.fd == NO_FD while we...
Posted by oknet <gi...@git.apache.org>.
Github user oknet closed the pull request at:
https://github.com/apache/trafficserver/pull/1033
---
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 #1033: TS-4879: Checking con.fd == NO_FD while we get re...
Posted by atsci <gi...@git.apache.org>.
Github user atsci commented on the issue:
https://github.com/apache/trafficserver/pull/1033
Linux build *successful*! See https://ci.trafficserver.apache.org/job/Github-Linux/748/ 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 #1033: TS-4879: Checking con.fd == NO_FD while we get re...
Posted by atsci <gi...@git.apache.org>.
Github user atsci commented on the issue:
https://github.com/apache/trafficserver/pull/1033
Linux build *successful*! See https://ci.trafficserver.apache.org/job/Github-Linux/735/ 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 #1033: TS-4879: Checking con.fd == NO_FD while we get re...
Posted by atsci <gi...@git.apache.org>.
Github user atsci commented on the issue:
https://github.com/apache/trafficserver/pull/1033
Linux build *successful*! See https://ci.trafficserver.apache.org/job/Github-Linux/747/ 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 pull request #1033: TS-4879: Checking con.fd == NO_FD while we...
Posted by jpeach <gi...@git.apache.org>.
Github user jpeach commented on a diff in the pull request:
https://github.com/apache/trafficserver/pull/1033#discussion_r79422585
--- Diff: iocore/net/UnixNetVConnection.cc ---
@@ -1273,14 +1273,21 @@ UnixNetVConnection::connectUp(EThread *t, int fd)
con.is_bound = true;
}
+ if (check_emergency_throttle(con)) {
+ // `con' could be closed if there is hyper emergency
+ if (con.fd == NO_FD) {
+ NET_SUM_GLOBAL_DYN_STAT(net_connections_currently_open_stat, -1);
+ // set errno force to EMFILE (reached limit for open file descriptors)
+ res = errno = EMFILE;
+ goto fail;
+ }
+ }
+
// Must connect after EventIO::Start() to avoid a race condition
// when edge triggering is used.
if (ep.start(get_PollDescriptor(t), this, EVENTIO_READ | EVENTIO_WRITE) < 0) {
- lerrno = errno;
Debug("iocore_net", "connectUp : Failed to add to epoll list");
- action_.continuation->handleEvent(NET_EVENT_OPEN_FAILED, (void *)0); // 0 == res
- free(t);
- return CONNECT_FAILURE;
+ goto fail;
--- End diff --
Set ``res``?
---
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 #1033: TS-4879: Checking con.fd == NO_FD while we get re...
Posted by atsci <gi...@git.apache.org>.
Github user atsci commented on the issue:
https://github.com/apache/trafficserver/pull/1033
FreeBSD build *successful*! See https://ci.trafficserver.apache.org/job/Github-FreeBSD/839/ 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 #1033: TS-4879: Checking con.fd == NO_FD while we get re...
Posted by jpeach <gi...@git.apache.org>.
Github user jpeach commented on the issue:
https://github.com/apache/trafficserver/pull/1033
@oknet I'd prefer 2 commits (better for bisecting).
---
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 #1033: TS-4879: Checking con.fd == NO_FD while we...
Posted by jpeach <gi...@git.apache.org>.
Github user jpeach commented on a diff in the pull request:
https://github.com/apache/trafficserver/pull/1033#discussion_r80158464
--- Diff: iocore/net/UnixNetVConnection.cc ---
@@ -1273,14 +1273,21 @@ UnixNetVConnection::connectUp(EThread *t, int fd)
con.is_bound = true;
}
+ if (check_emergency_throttle(con)) {
+ // `con' could be closed if there is hyper emergency
+ if (con.fd == NO_FD) {
+ NET_SUM_GLOBAL_DYN_STAT(net_connections_currently_open_stat, -1);
+ // set errno force to EMFILE (reached limit for open file descriptors)
+ res = errno = EMFILE;
+ goto fail;
+ }
+ }
+
// Must connect after EventIO::Start() to avoid a race condition
// when edge triggering is used.
if (ep.start(get_PollDescriptor(t), this, EVENTIO_READ | EVENTIO_WRITE) < 0) {
- lerrno = errno;
Debug("iocore_net", "connectUp : Failed to add to epoll list");
- action_.continuation->handleEvent(NET_EVENT_OPEN_FAILED, (void *)0); // 0 == res
- free(t);
- return CONNECT_FAILURE;
+ goto fail;
--- End diff --
I think it would be more correct to set ``res`` to ``-errno``. Ultimately, ``EventIO::start`` returns -1 on failure and the other parts of this code are setting ``res`` to ``-errno`` on failure.
How about:
```C
if (ep.start(get_PollDescriptor(t), this, EVENTIO_READ | EVENTIO_WRITE) < 0) {
res = -errno;
Debug("iocore_net", "failed to add to epoll list: %s", strerror(errno));
goto fail;
}
```
---
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 #1033: TS-4879: Checking con.fd == NO_FD while we get re...
Posted by atsci <gi...@git.apache.org>.
Github user atsci commented on the issue:
https://github.com/apache/trafficserver/pull/1033
FreeBSD build *successful*! See https://ci.trafficserver.apache.org/job/Github-FreeBSD/852/ 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 #1033: TS-4879: Checking con.fd == NO_FD while we get re...
Posted by jpeach <gi...@git.apache.org>.
Github user jpeach commented on the issue:
https://github.com/apache/trafficserver/pull/1033
@oknet nice!
---
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.
---