Skip to content

Do not callback one SM EVENT_ERROR twice.#1559

Merged
zwoop merged 1 commit intoapache:masterfrom
oknet:i1559
Mar 9, 2017
Merged

Do not callback one SM EVENT_ERROR twice.#1559
zwoop merged 1 commit intoapache:masterfrom
oknet:i1559

Conversation

@oknet
Copy link
Copy Markdown
Member

@oknet oknet commented Mar 9, 2017 •
edited
Loading

After #947 (c1ac5f) and #1522 (a128d5) , the EVENT_ERROR which caused by
EPOLLERR will be sent to read.vio._cont first and then write.vio._cont.

The reader SM could close or shutdown(WRITE) the VC, but we do not check
these operations before we callback write.vio._cont. The SM would
received EVENT_ERROR twice if write.vio._cont == read.vio._cont.

@oknet oknet force-pushed the i1559 branch 2 times, most recently from 988d100 to f0c816f Compare March 9, 2017 05:25
@oknet
Do not callback one SM EVENT_ERROR twice.
121f592 
After apache#947 (c1ac5f) and apache#1522 (a128d5) , the EVENT_ERROR which caused by
EPOLLERR will be sent to read.vio._cont first and then write.vio._cont.

The reader SM could close or shutdown(WRITE) the VC, but we do not check
these operations before we callback write.vio._cont. The SM would
received EVENT_ERROR twice if write.vio._cont == read.vio._cont.
@atsci
Copy link
Copy Markdown

atsci commented Mar 9, 2017

clang format successful! https://ci.trafficserver.apache.org/job/clang-format-github/20/

@atsci
Copy link
Copy Markdown

atsci commented Mar 9, 2017

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

@atsci
Copy link
Copy Markdown

atsci commented Mar 9, 2017

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

@atsci
Copy link
Copy Markdown

atsci commented Mar 9, 2017

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

@atsci
Copy link
Copy Markdown

atsci commented Mar 9, 2017

AU check failed! https://ci.trafficserver.apache.org/job/autest-github/19/

@atsci
Copy link
Copy Markdown

atsci commented Mar 9, 2017

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

@oknet
Copy link
Copy Markdown
Member Author

oknet commented Mar 9, 2017

This is a fix for #1531 to avoid enforce the handler to handle read error and write error.

@scw00 Can you have a try ?

@zwoop May I ignore the AU check failed ?

@atsci
Copy link
Copy Markdown

atsci commented Mar 9, 2017

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

@atsci
Copy link
Copy Markdown

atsci commented Mar 9, 2017

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

@scw00
Copy link
Copy Markdown
Member

scw00 commented Mar 9, 2017

@oknet I it work well on my test env, and jtest can not re-product any more !

@oknet oknet requested review from bryancall and zwoop March 9, 2017 09:20
@oknet oknet self-assigned this Mar 9, 2017
@oknet oknet added the Core label Mar 9, 2017
@oknet oknet added this to the 7.2.0 milestone Mar 9, 2017
@oknet oknet mentioned this pull request Mar 9, 2017
Closed
@zwoop
Copy link
Copy Markdown
Contributor

zwoop commented Mar 9, 2017

@oknet Yes, ignore AU tests for now.

@zwoop
Copy link
Copy Markdown
Contributor

zwoop commented Mar 9, 2017 •
edited
Loading

Nice! Testing this PR on docs.trafficserver now. Remember to close #1531 after we land this :).

zwoop
zwoop approved these changes Mar 9, 2017
View reviewed changes
Comment thread iocore/net/UnixNetVConnection.cc
return;
}
// If vc is closed or shutdown(WRITE) in last read_signal_error callback,
// or reader_cont is same as write.vio._cont.
Copy link
Copy Markdown
Contributor

@zwoop zwoop Mar 9, 2017

Choose a reason for hiding this comment

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

Great comment, this really helps explaining the issue / solution.

@zwoop zwoop modified the milestones: 7.1.0, 7.2.0 Mar 9, 2017
@zwoop zwoop merged commit aee3f3b into apache:master Mar 9, 2017
@oknet oknet mentioned this pull request Mar 13, 2017
Closed
@scw00 scw00 mentioned this pull request Jun 1, 2017
Closed
vmamidi added a commit to vmamidi/trafficserver that referenced this pull request Jun 8, 2017
@vmamidi
Revert "TS-4796 Change UnixNetHandler to always bubble up epoll error…
92102b7 
…s to the VConnection"

this reverts PRs apache#1559, apache#1522 and apache#947

This reverts commit c1ac5f8.
@vmamidi vmamidi mentioned this pull request Jun 8, 2017
Merged
vmamidi added a commit that referenced this pull request Jun 8, 2017
@vmamidi
Revert "TS-4796 Change UnixNetHandler to always bubble up epoll error…
4d9279c 
…s to the VConnection"

this reverts PRs #1559, #1522 and #947

This reverts commit c1ac5f8.
vmamidi added a commit to vmamidi/trafficserver that referenced this pull request Jun 8, 2017
@vmamidi
Revert PR apache#947 - merge to 7.1.x
6159117 
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.
@vmamidi vmamidi mentioned this pull request Jun 8, 2017
Merged
zwoop pushed a commit that referenced this pull request Jun 8, 2017
@vmamidi @zwoop
Revert PR #947 - merge to 7.1.x
65a001b 
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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@zwoop zwoop zwoop approved these changes

@bryancall bryancall bryancall approved these changes

Assignees

@oknet oknet

Labels

Core

Projects

None yet

Milestone

7.1.0

Development

Successfully merging this pull request may close these issues.

5 participants

@oknet @atsci @scw00 @zwoop @bryancall