don't crash when Channel goes inactive in read triggered by write error #594
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
weissi
requested review from
Lukasa and
normanmaurer
and removed request for
Lukasa
weissi
commented
Aug 23, 2018
| var error: ChannelError { | ||
| switch self { | ||
| case .all: | ||
| return .alreadyClosed | ||
| return .ioOnClosedChannel |
There was a problem hiding this comment.
this was just the wrong error here, .alreadyClosed means close on previously closed channel. For all others, .ioOnClosedChannel is right
weissi
force-pushed
the
jw-chan-inactive-write-error
branch
from
1fc982b
to
30fce55
Compare
weissi
force-pushed
the
jw-chan-inactive-write-error
branch
2 times, most recently
from
06c4eae
to
271b6fd
Compare
vlm
reviewed
Aug 23, 2018
Sources/NIO/BaseSocketChannel.swift
Outdated
| assert(self.lifecycleManager.isActive) | ||
| pipeline.fireChannelReadComplete() | ||
| if self.lifecycleManager.isActive { | ||
| pipeline.fireChannelReadComplete() |
There was a problem hiding this comment.
No, this is thread-safe.
I think a more important question is why we fire readComplete more than once. I feel like we shouldn't be doing that: we should probably move the fireChannelReadComplete outside the while loop.
There was a problem hiding this comment.
@Lukasa I have no idea either why we're doing that but left it because it was that way. But let me just change it
There was a problem hiding this comment.
@vlm thread-safety is fine here because we're on the Channel's event loop. So (unless we call out), the value of self.lifecycleManager.isActive can't change. So if it's still active after readFromSocket returns, it'll remain active until we return or call out again.
Motivation: Previously we asserted that a Channel cannot go inactive after calling out for a read that was triggered by (draining the receive buffer after) a write error. This assertion was put in place to guard against `readComplete` events sent on inactive channels. It did that job just fine but crashes aren't great so we now conditionally fire the `readComplete` event if the Channel stays active. Modifications: make the readComplete event firing conditional Result: fewer crashes, more happy faces
weissi
force-pushed
the
jw-chan-inactive-write-error
branch
from
271b6fd
to
57eb1f4
Compare
weissi
added
the
patch-version-bump-only
|
CI seriously unhappy: https://ci.swiftnio.io/job/swift-nio-master-410-prb/519/console |
|
@tomerd ^^^ |
|
@swift-nio-bot test this please |
|
yea looks like ci broke, will look into it |
|
thanks @tomerd |
|
ugh, the ebs volume got detached. working to fix |
|
@swift-nio-bot test this please |
|
@weissi should be okay now, monitoring too |
|
awesome, thanks so much @tomerd |
|
also hit the confused kernel which is #563 |
|
@swift-nio-bot test this please |
weissi
added a commit
that referenced
this pull request
Aug 29, 2018
…or (#594) Motivation: Previously we asserted that a Channel cannot go inactive after calling out for a read that was triggered by (draining the receive buffer after) a write error. This assertion was put in place to guard against `readComplete` events sent on inactive channels. It did that job just fine but crashes aren't great so we now conditionally fire the `readComplete` event if the Channel stays active. Modifications: make the readComplete event firing conditional Result: fewer crashes, more happy faces Motivation: Explain here the context, and why you're making that change. What is the problem you're trying to solve. Modifications: Describe the modifications you've done. Result: After your change, what will change.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Motivation:
Previously we asserted that a Channel cannot go inactive after calling
out for a read that was triggered by (draining the receive buffer after)
a write error.
This assertion was put in place to guard against
readCompleteeventssent on inactive channels. It did that job just fine but crashes aren't
great so we now conditionally fire the
readCompleteevent if theChannel stays active.
Modifications:
make the readComplete event firing conditional
Result:
fewer crashes, more happy faces