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

platform/unix: Don't return ECONNRESET if dedicated channel is closed #148

Closed
wants to merge 1 commit into from

Conversation

@antrik
Copy link
Contributor

antrik commented Feb 14, 2017

So far, when the sender of a dedicated channel in a large transfer was
closed before the receiver got the expected amount of data, recv()
returned a "channel closed" status, just as if the main channel was
closed. This is incorrect though, since the main channel is still open,
and the receiver shouldn't be dropped.

Returning EIO now instead -- which is a bit of an abuse I guess; but
probably as good a choice as any, as long as we are sticking with Unix
error codes only...

So far, when the sender of a dedicated channel in a large transfer was
closed before the receiver got the expected amount of data, `recv()`
returned a "channel closed" status, just as if the main channel was
closed. This is incorrect though, since the main channel is still open,
and the receiver shouldn't be dropped.

Returning `EIO` now instead -- which is a bit of an abuse I guess; but
probably as good a choice as any, as long as we are sticking with Unix
error codes only...
@antrik
Copy link
Contributor Author

antrik commented Feb 14, 2017

I'm not too happy with this: while I believe that it is more correct, I don't know yet how to verify that it actually improves anything...

@nox could you do another try run, with this instead of #147 ? Not sure it will change anything -- but I guess it's worth a shot.

@nox
Copy link
Member

nox commented Feb 14, 2017

@antrik Sure!

@antrik
Copy link
Contributor Author

antrik commented Feb 15, 2017

To clarify: while it turns out #133 is not affected by this at all, I'm still pretty sure it's more correct this way, and we should commit the change...

@nox
Copy link
Member

nox commented Feb 15, 2017

r? @danlrobertson

@dlrobertson
Copy link
Collaborator

dlrobertson commented Feb 15, 2017

Is this still valid? It seems like a possible scenario, and I did run a test with #149 and this that ran without issues.

@antrik
Copy link
Contributor Author

antrik commented Feb 15, 2017

It turns out I'm not so sure any more now...

The only scenario I can think of where this would get triggered, is when the sender dies unexpectedly before it finishes the send. Since the main channel normally would also get closed in this case, returning ECONNRESET would not actually be wrong... Except that there might be another process still having a sender, in which case it would be definitely wrong. So this probably needs some kind of case handling... Let me ponder it a bit more.

Closing for now to avoid confusion.

@antrik antrik closed this Feb 15, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

5 participants
You can’t perform that action at this time.