Skip to content
This repository was archived by the owner on Mar 11, 2020. It is now read-only.
This repository was archived by the owner on Mar 11, 2020. It is now read-only.

transport: Write errors not handled as fatal #16

Description

@kennylevinsen

Errors when writing messages are handled as non-fatal at the current state, letting the client make the decisions. A failed write is only okay if no data was written - if any data was written, the protocol is trashed and the connection must be let go. If the error is not handled as fatal, and another write is made (could have been queued already), the half-written message + the new message might be read as a valid message by the server which will then do something unexpected, such as responding to a tag the client never used, or writing random data to fids.

Activity

stevvooe commented on Aug 12, 2016

@stevvooe
Contributor

@Joushou Could you send a PR that makes this fix?

kennylevinsen commented on Aug 16, 2016

@kennylevinsen
ContributorAuthor

Not sure if I can allocate the time right now, but the PR should be fairly easy to make. I also doubt that it's super high priority - normally, if a write fails, so will the next, and temporary errors are okay as long as no data was written, making problems unlikely. The issue is more about proper design.

stevvooe commented on Aug 16, 2016

@stevvooe
Contributor

For the most part, this was written to be fatal on any kind of error, so I am not sure what specifically you want changed.

A few questions:

Where in the code do you not see this as the case?
What changes would you like to see happen?
Is the problem that we aren't classifying temporary vs fatal errors or do you want the session to be poisoned after encountering an error?
Should this happen to both the client and the server?

stevvooe commented on Aug 19, 2016

@stevvooe
Contributor

@Joushou Found this little gem:

// BUG(stevvooe): There may be partial reads under
// timeout errors where this is actually fatal.

I'm not sure what the right fix here is. Let's fill out some unit test coverage in this code and reproduce these hypothetical situations. That will allow us to find the right error handling.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions