Skip to content

Conversation

@scw00
Copy link
Member

@scw00 scw00 commented Apr 3, 2017

In my opinion, the EPOLLERR always with read.enabled or write.enabled(otherwise, there is no sm could handle this event, we just ignore). So when EPOLLERR happens, we could push the vc into read_ready_list if the read.enabled == 1, otherwise push it into write_ready_list.

@zwoop zwoop added the Network label Apr 3, 2017
@zwoop zwoop added this to the 7.2.0 milestone Apr 3, 2017
@zwoop
Copy link
Contributor

zwoop commented Apr 3, 2017

[approve ci]

@zwoop zwoop requested a review from oknet April 3, 2017 15:13
@atsci
Copy link

atsci commented Apr 3, 2017

@atsci
Copy link

atsci commented Apr 3, 2017

@atsci
Copy link

atsci commented Apr 3, 2017

@atsci
Copy link

atsci commented Apr 3, 2017

FreeBSD11 build failed! https://ci.trafficserver.apache.org/job/freebsd-github/1821/

@atsci
Copy link

atsci commented Apr 3, 2017

clang-analyzer build failed! https://ci.trafficserver.apache.org/job/clang-analyzer-github/383/

@atsci
Copy link

atsci commented Apr 3, 2017

Intel CC build failed! https://ci.trafficserver.apache.org/job/icc-github/251/

@atsci
Copy link

atsci commented Apr 3, 2017

@oknet
Copy link
Member

oknet commented Apr 5, 2017

@scw00 Can you have a try with only these 2 lines change?

diff --git a/iocore/net/UnixNet.cc b/iocore/net/UnixNet.cc
index c7f1798..2f98269 100644
--- a/iocore/net/UnixNet.cc
+++ b/iocore/net/UnixNet.cc
@@ -450,7 +450,7 @@ NetHandler::mainNetEvent(int event, Event *e)
       if (cop_list.in(vc)) {
         cop_list.remove(vc);
       }
-      if (get_ev_events(pd, x) & EVENTIO_READ) {
+      if (get_ev_events(pd, x) & (EVENTIO_READ | EVENTIO_ERROR)) {
         vc->read.triggered = 1;
         if (get_ev_events(pd, x) & EVENTIO_ERROR) {
           vc->read.error = 1;
@@ -466,7 +466,7 @@ NetHandler::mainNetEvent(int event, Event *e)
         }
       }
       vc = epd->data.vc;
-      if (get_ev_events(pd, x) & EVENTIO_WRITE) {
+      if (get_ev_events(pd, x) & (EVENTIO_WRITE | EVENTIO_ERROR)) {
         vc->write.triggered = 1;
         if (get_ev_events(pd, x) & EVENTIO_ERROR) {
           vc->write.error = 1;

To resolve the issue we just revert these 2 lines to the state #947 .

get_ev_events(pd, x), vc->write.enabled, vc->closed, write_ready_list.in(vc));
if (!write_ready_list.in(vc)) {
write_ready_list.enqueue(vc);
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You just ignore the error if enabled==0. This is equal to revert #947 .
I think this is not a good fix to the issue.

@scw00
Copy link
Member Author

scw00 commented Apr 5, 2017

Based on @oknet , Here is a new logic. We just handle write error when read error was handled, or there is only a write error.

@oknet
Copy link
Member

oknet commented Apr 5, 2017

[approve ci]

@oknet
Copy link
Member

oknet commented Apr 5, 2017

Oops ... I don't have permission to execute CI. @zwoop Can I have "approve ci" ? :-D

@oknet
Copy link
Member

oknet commented Apr 8, 2017

@zwoop Can you have a try on docs ?

@zwoop
Copy link
Contributor

zwoop commented Apr 12, 2017

[approve ci]

@atsci
Copy link

atsci commented Apr 12, 2017

@atsci
Copy link

atsci commented Apr 12, 2017

@atsci
Copy link

atsci commented Apr 12, 2017

RAT check successful! https://ci.trafficserver.apache.org/job/RAT-github/184/

@atsci
Copy link

atsci commented Apr 12, 2017

@atsci
Copy link

atsci commented Apr 12, 2017

Intel CC build failed! https://ci.trafficserver.apache.org/job/icc-github/295/

@atsci
Copy link

atsci commented Apr 12, 2017

FreeBSD11 build failed! https://ci.trafficserver.apache.org/job/freebsd-github/1865/

@atsci
Copy link

atsci commented Apr 12, 2017

clang-analyzer build successful! https://ci.trafficserver.apache.org/job/clang-analyzer-github/428/

@zwoop
Copy link
Contributor

zwoop commented Apr 12, 2017

@oknet I've added you as admin of all the builds.

@zwoop
Copy link
Contributor

zwoop commented Apr 12, 2017

Looks like there's a build failure here:

UnixNetVConnection.cc: In function 'void write_to_net_io(NetHandler*, UnixNetVConnection*, EThread*)':
UnixNetVConnection.cc:472:39: error: 'struct NetState' has no member named '_cont'
       if (!vc->read.error || vc->read._cont == nullptr) {
                                       ^

@oknet
Copy link
Member

oknet commented Apr 13, 2017

[approve ci]

@atsci
Copy link

atsci commented Apr 13, 2017

@atsci
Copy link

atsci commented Apr 13, 2017

RAT check successful! https://ci.trafficserver.apache.org/job/RAT-github/190/

@atsci
Copy link

atsci commented Apr 13, 2017

@atsci
Copy link

atsci commented Apr 13, 2017

Intel CC build successful! https://ci.trafficserver.apache.org/job/icc-github/301/

@atsci
Copy link

atsci commented Apr 13, 2017

Linux build successful! https://ci.trafficserver.apache.org/job/linux-github/1761/

@atsci
Copy link

atsci commented Apr 13, 2017

FreeBSD11 build successful! https://ci.trafficserver.apache.org/job/freebsd-github/1871/

@atsci
Copy link

atsci commented Apr 13, 2017

clang-analyzer build successful! https://ci.trafficserver.apache.org/job/clang-analyzer-github/434/

@oknet
Copy link
Member

oknet commented Apr 13, 2017

@zwoop Please test this PR on docs. thanks. @scw00 have tested it by a python script.

@oknet
Copy link
Member

oknet commented Jun 5, 2017

[approve ci] @scw00 Can you write a autest script ?

@vmamidi vmamidi mentioned this pull request Jun 8, 2017
@bryancall
Copy link
Contributor

This looks like it is related to #947

@zwoop
Copy link
Contributor

zwoop commented Jun 8, 2017

Since we're reverting #947, I'm closing this (please reopen if that is not right).

@zwoop zwoop closed this Jun 8, 2017
vmamidi added a commit to vmamidi/trafficserver that referenced this pull request Jun 8, 2017
This reverts PRs apache#1559, apache#1522 and apache#947

PR apache#947 made the HTTP state machine unstable and lead to crashes in production like apache#1930 apache#1559 apache#1522 apache#1531 apache#1629

This reverts commit c1ac5f8.
zwoop pushed a commit that referenced this pull request Jun 8, 2017
This reverts PRs #1559, #1522 and #947

PR #947 made the HTTP state machine unstable and lead to crashes in production like #1930 #1559 #1522 #1531 #1629

This reverts commit c1ac5f8.
@zwoop zwoop modified the milestone: 8.0.0 Jul 7, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants