You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@trafficserver.apache.org by oknet <gi...@git.apache.org> on 2016/06/12 13:20:27 UTC
[GitHub] trafficserver pull request #701: TS-4522: check EPIPE instead r==0
GitHub user oknet opened a pull request:
https://github.com/apache/trafficserver/pull/701
TS-4522: check EPIPE instead r==0
man 2 write, Return value, On Success, the number of bytes written is returned (zero indicates nothing was written). On error, -1 is returned, and errno is set appropriately.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/oknet/trafficserver patch-14
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/trafficserver/pull/701.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 #701
----
commit d381a2d4ea5511696484f278b9aa80e4bdea545f
Author: Oknet <xu...@gmail.com>
Date: 2016-06-12T13:08:47Z
TS-4522: check EPIPE instead r==0
man 2 write, Return value, On Success, the number of bytes written is returned (zero indicates nothing was written). On error, -1 is returned, and errno is set appropriately.
----
---
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 #701: TS-4522: Should signal SM with EVENT_ERROR on erro...
Posted by atsci <gi...@git.apache.org>.
Github user atsci commented on the issue:
https://github.com/apache/trafficserver/pull/701
Linux build *successful*! See https://ci.trafficserver.apache.org/job/Github-Linux/759/ 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 #701: TS-4522: check EPIPE instead r==0
Posted by oknet <gi...@git.apache.org>.
Github user oknet commented on the issue:
https://github.com/apache/trafficserver/pull/701
@jpeach ECONNRESET is only found in man 2 send/sendto/sendmsg. obviously EPIPE for write() the same as ECONNRESET for send()/sendto()/sendmsg(). And write() is called in load_buffer_and_write().
---
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 #701: TS-4522: Should signal SM with EVENT_ERROR ...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/trafficserver/pull/701
---
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 #701: TS-4522: Should signal SM with EVENT_ERROR on erro...
Posted by atsci <gi...@git.apache.org>.
Github user atsci commented on the issue:
https://github.com/apache/trafficserver/pull/701
FreeBSD build *successful*! See https://ci.trafficserver.apache.org/job/Github-FreeBSD/863/ 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 #701: TS-4522: Should signal SM with EVENT_ERROR on erro...
Posted by atsci <gi...@git.apache.org>.
Github user atsci commented on the issue:
https://github.com/apache/trafficserver/pull/701
FreeBSD build *successful*! See https://ci.trafficserver.apache.org/job/Github-FreeBSD/864/ 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 #701: TS-4522: check EPIPE instead r==0
Posted by atsci <gi...@git.apache.org>.
Github user atsci commented on the issue:
https://github.com/apache/trafficserver/pull/701
Linux build *failed*! See https://ci.trafficserver.apache.org/job/Github-Linux/120/ 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 #701: TS-4522: check EPIPE instead r==0
Posted by jpeach <gi...@git.apache.org>.
Github user jpeach commented on the issue:
https://github.com/apache/trafficserver/pull/701
AFAICT we would get ``EPIPE`` when we write to a socket that has been closed. This would be an error because there is unwritten data that we can't write. We would get ``ECONNRESET`` reading from a socket that is closed. The other side closed the socket but we don't know whether that was premature or not (yet). Most places that I saw treated ``VC_EVENT_ERROR`` and ``VC_EVENT_EOS`` similarly apart from logging.
By the same logic, I agree that handling ``ECONNRESET`` specially in ``write_to_net_io`` looks odd and could be a bug. Maybe the right change is to remove the ``ECONNRESET`` check in ``write_to_net_io``. Also in this code path, the return from ``UnixNetVConnection::load_buffer_and_write`` should never be 0 because we never try to write 0 bytes.
So consider:
```C
diff --git a/iocore/net/UnixNetVConnection.cc b/iocore/net/UnixNetVConnection.cc
index b52985c..bc9764d 100644
--- a/iocore/net/UnixNetVConnection.cc
+++ b/iocore/net/UnixNetVConnection.cc
@@ -535,11 +535,9 @@ write_to_net_io(NetHandler *nh, UnixNetVConnection *vc, EThread *thread)
}
return;
}
- if (!r || r == -ECONNRESET) {
- vc->write.triggered = 0;
- write_signal_done(VC_EVENT_EOS, nh, vc);
- return;
- }
+ // A write of 0 makes no sense since we tried to write more than 0. Either
+ // we wrote something or we got an error.
+ ink_assert(r < 0);
vc->write.triggered = 0;
write_signal_error(nh, vc, (int)-total_written);
return;
```
---
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 #701: TS-4522: Should signal SM with EVENT_ERROR on erro...
Posted by atsci <gi...@git.apache.org>.
Github user atsci commented on the issue:
https://github.com/apache/trafficserver/pull/701
Linux build *successful*! See https://ci.trafficserver.apache.org/job/Github-Linux/760/ 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 #701: TS-4522: Should signal SM with EVENT_ERROR on erro...
Posted by atsci <gi...@git.apache.org>.
Github user atsci commented on the issue:
https://github.com/apache/trafficserver/pull/701
Linux build *successful*! See https://ci.trafficserver.apache.org/job/Github-Linux/758/ 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 #701: TS-4522: check EPIPE instead r==0
Posted by atsci <gi...@git.apache.org>.
Github user atsci commented on the issue:
https://github.com/apache/trafficserver/pull/701
Linux build *successful*! See https://ci.trafficserver.apache.org/job/Github-Linux/121/ 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 #701: TS-4522: check EPIPE instead r==0
Posted by atsci <gi...@git.apache.org>.
Github user atsci commented on the issue:
https://github.com/apache/trafficserver/pull/701
FreeBSD build *successful*! See https://ci.trafficserver.apache.org/job/Github-FreeBSD/224/ 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 #701: TS-4522: check EPIPE instead r==0
Posted by zwoop <gi...@git.apache.org>.
Github user zwoop commented on the issue:
https://github.com/apache/trafficserver/pull/701
@oknet How do you want to proceed with this? Are you making an update to the PR to address some of the review suggestions?
---
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 #701: TS-4522: check EPIPE instead r==0
Posted by oknet <gi...@git.apache.org>.
Github user oknet commented on the issue:
https://github.com/apache/trafficserver/pull/701
I'm try do some test and get result below:
ECONNRESET from write() while get TCP_RST from peer.
EPIPE from write() while get TCP_FIN from peer.
SSLNetVConnection::load_buffer_and_write() would return 0 on SSL_ERROR_NONE, but I believe SSL_ERROR_NONE never occur on SSLBufferWrite().
---
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 #701: TS-4522: check EPIPE instead r==0
Posted by zwoop <gi...@git.apache.org>.
Github user zwoop commented on the issue:
https://github.com/apache/trafficserver/pull/701
Jenkins workspaces in disarray due to previous failures (sigh), trying again. [approve ci].
---
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 #701: TS-4522: check EPIPE instead r==0
Posted by shinrich <gi...@git.apache.org>.
Github user shinrich commented on the issue:
https://github.com/apache/trafficserver/pull/701
@oknet the SSL_write logic is handled in SSLUtils/SSLNetVConnection. The openssl library does its own read/write for the most part, and that logic handles the SSL_ERROR_WANT_READ case. In the SSL handshake we rely on UnixNetVConnection for reads, but not writes.
---
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 #701: TS-4522: check EPIPE instead r==0
Posted by jpeach <gi...@git.apache.org>.
Github user jpeach commented on the issue:
https://github.com/apache/trafficserver/pull/701
This begs the question of why ``-ECONNRESET`` is treated as ``VC_EVENT_EOS`` rather than ``VC_EVENT_ERROR``.
---
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 #701: TS-4522: Should signal SM with EVENT_ERROR on erro...
Posted by oknet <gi...@git.apache.org>.
Github user oknet commented on the issue:
https://github.com/apache/trafficserver/pull/701
Copy&Paste bug in load_buffer_and_write() if 0 returned from write() or writev().
---
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 #701: TS-4522: Should signal SM with EVENT_ERROR on erro...
Posted by zwoop <gi...@git.apache.org>.
Github user zwoop commented on the issue:
https://github.com/apache/trafficserver/pull/701
Status on this?
---
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 #701: TS-4522: check EPIPE instead r==0
Posted by oknet <gi...@git.apache.org>.
Github user oknet commented on the issue:
https://github.com/apache/trafficserver/pull/701
@jpeach VC_EVENT_EOS indicates socket fd is closed. VC_EVENT_ERROR means meet a error and can not going on. Looking around the comment on I_VConnection.h about do_io_read and do_io_write. VC_EVENT_EOS is not introduced in do_io_write, only VC_EVENT_ERROR.
Is VC_EVENT_EOS callbacked only do_io_read first ?
Write to a closed fd then get -EPIPE and VC_EVENT_ERROR callbacked, is it right ?
---
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 #701: TS-4522: check EPIPE instead r==0
Posted by shinrich <gi...@git.apache.org>.
Github user shinrich commented on the issue:
https://github.com/apache/trafficserver/pull/701
I think that I agree with @jpeach's comments and code suggestion. Returning _EOS really only makes sense for read. I think by the time you get to that point in the code it is a failure.
---
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 #701: TS-4522: Should signal SM with EVENT_ERROR on erro...
Posted by oknet <gi...@git.apache.org>.
Github user oknet commented on the issue:
https://github.com/apache/trafficserver/pull/701
change the assert condition from (r==0) to (r!=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 issue #701: TS-4522: check EPIPE instead r==0
Posted by oknet <gi...@git.apache.org>.
Github user oknet commented on the issue:
https://github.com/apache/trafficserver/pull/701
agree with @jpeach 's comments and code suggestion. With minor modify that only assert on ( r == 0 ) because of (r < 0) means error on write().
---
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 #701: TS-4522: check EPIPE instead r==0
Posted by zwoop <gi...@git.apache.org>.
Github user zwoop commented on the issue:
https://github.com/apache/trafficserver/pull/701
[approve ci]
---
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 #701: TS-4522: Should signal SM with EVENT_ERROR on erro...
Posted by atsci <gi...@git.apache.org>.
Github user atsci commented on the issue:
https://github.com/apache/trafficserver/pull/701
FreeBSD build *successful*! See https://ci.trafficserver.apache.org/job/Github-FreeBSD/862/ 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 #701: TS-4522: check EPIPE instead r==0
Posted by atsci <gi...@git.apache.org>.
Github user atsci commented on the issue:
https://github.com/apache/trafficserver/pull/701
FreeBSD build *failed*! See https://ci.trafficserver.apache.org/job/Github-FreeBSD/223/ 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 #701: TS-4522: check EPIPE instead r==0
Posted by oknet <gi...@git.apache.org>.
Github user oknet commented on the issue:
https://github.com/apache/trafficserver/pull/701
@shinrich I remember that SSL_write would return \u201cSSL_WANT_READ\u201d error code.
from: https://www.openssl.org/docs/manmaster/ssl/SSL_write.html
If the underlying BIO is non-blocking, SSL_write() will also return, when the underlying BIO could not satisfy the needs of SSL_write() to continue the operation. In this case a call to SSL_get_error with the return value of SSL_write() will yield SSL_ERROR_WANT_READ or SSL_ERROR_WANT_WRITE. As at any time a re-negotiation is possible, a call to SSL_write() can also cause read operations! The calling process then must repeat the call after taking appropriate action to satisfy the needs of SSL_write(). The action depends on the underlying BIO. When using a non-blocking socket, nothing is to be done, but select() can be used to check for the required condition. When using a buffering BIO, like a BIO pair, data must be written into or retrieved out of the BIO before being able to continue.
Is the code added for handle SSL_ERROR_WANT_READ or related ?
---
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 #701: TS-4522: check EPIPE instead r==0
Posted by zwoop <gi...@git.apache.org>.
Github user zwoop commented on the issue:
https://github.com/apache/trafficserver/pull/701
@oknet ping ?
---
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 #701: TS-4522: Should signal SM with EVENT_ERROR on erro...
Posted by oknet <gi...@git.apache.org>.
Github user oknet commented on the issue:
https://github.com/apache/trafficserver/pull/701
@jpeach Could you please review this PR? I think this is final version.
---
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.
---