Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Do not lose data on channel close. #40

Merged
merged 1 commit into from
Aug 17, 2020
Merged

Do not lose data on channel close. #40

merged 1 commit into from
Aug 17, 2020

Conversation

Lukasa
Copy link
Collaborator

@Lukasa Lukasa commented Aug 17, 2020

Motivation:

We didn't notice this as much when we had autoRead turned on and the
broken data fast path, but now that we don't it has become apparent that
we can see data loss if CHANNEL_CLOSE arrives while we're queueing
reads. That's not good!

Here we decide to adopt the same pattern as NIO does with TCP channels:
if we receive a hard-close, we will deliver all pending data, even if
you have not got an outstanding read(). This avoids data loss, while
allowing us to rapidly drop resources that are potentially in short
supply. In this case, if the remote peer doesn't care about this channel
anymore, we should really stop holding it open ourselves.

Modifications:

  • No longer drop reads in any circumstance, always deliver them.
  • Wrote some tests.

Result:

No data loss!

Motivation:

We didn't notice this as much when we had autoRead turned on and the
broken data fast path, but now that we don't it has become apparent that
we can see data loss if CHANNEL_CLOSE arrives while we're queueing
reads. That's not good!

Here we decide to adopt the same pattern as NIO does with TCP channels:
if we receive a hard-close, we will deliver all pending data, even if
you have not got an outstanding read(). This avoids data loss, while
allowing us to rapidly drop resources that are potentially in short
supply. In this case, if the remote peer doesn't care about this channel
anymore, we should really stop holding it open ourselves.

Modifications:

- No longer drop reads in any circumstance, always deliver them.
- Wrote some tests.

Result:

No data loss!
@Lukasa Lukasa added the patch-version-bump-only For PRs that when merged will only cause a bump of the patch version, ie. 1.0.x -> 1.0.(x+1) label Aug 17, 2020
@Joannis
Copy link
Collaborator

Joannis commented Aug 17, 2020

Does the same expectation apply to the parent channel I.E. the SSH protocol itself? I'd say it's an expectation. A search of mine doesn't reveal any such tests so far.

@Lukasa
Copy link
Collaborator Author

Lukasa commented Aug 17, 2020

The answer is yes it does. There is less risk here because the NIOSSHHandler does not buffer inbound messages and must deliver them in order. As a result, the only way this can become a problem is if we re-entrantly receive channelInactive, which we should not do in a well-functioning pipeline. In those cases, we can see data loss, but we also cannot do anything about it because we didn't know about the data to begin with.

@Lukasa Lukasa merged commit e6ba385 into apple:main Aug 17, 2020
@Lukasa Lukasa deleted the cb-queue-eof branch August 17, 2020 15:40
artemredkin pushed a commit to artemredkin/swift-nio-ssh that referenced this pull request Feb 19, 2021
Motivation:

We didn't notice this as much when we had autoRead turned on and the
broken data fast path, but now that we don't it has become apparent that
we can see data loss if CHANNEL_CLOSE arrives while we're queueing
reads. That's not good!

Here we decide to adopt the same pattern as NIO does with TCP channels:
if we receive a hard-close, we will deliver all pending data, even if
you have not got an outstanding read(). This avoids data loss, while
allowing us to rapidly drop resources that are potentially in short
supply. In this case, if the remote peer doesn't care about this channel
anymore, we should really stop holding it open ourselves.

Modifications:

- No longer drop reads in any circumstance, always deliver them.
- Wrote some tests.

Result:

No data loss!
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
patch-version-bump-only For PRs that when merged will only cause a bump of the patch version, ie. 1.0.x -> 1.0.(x+1)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants