Skip to content

More context for EOF during client setup#513

Merged
drakkan merged 2 commits into
masterfrom
better-client-startup-diagnostic-messages
Jul 11, 2022
Merged

More context for EOF during client setup#513
drakkan merged 2 commits into
masterfrom
better-client-startup-diagnostic-messages

Conversation

@puellanivis

Copy link
Copy Markdown
Collaborator

Addresses concerns raised from #506 where a bare EOF was being returned, and thus being quite unclear about where the EOF happened.

This adds more context to the returned errors, and replaces EOF with UnexpectedEOF since we definitely aren’t expecting one.

However, I’m thinking a better message should be returned than even unexpected EOF. I’ll be digging through documentation to see what the best option here is.

@puellanivis

Copy link
Copy Markdown
Collaborator Author

The secsh-filexfer-02 document is silent about what to do in this case. I think however the error "server unexpectedly closed connection" might be even more clear than an unexpected EOF.

@drakkan

drakkan commented Jun 30, 2022

Copy link
Copy Markdown
Collaborator

The secsh-filexfer-02 document is silent about what to do in this case. I think however the error "server unexpectedly closed connection" might be even more clear than an unexpected EOF.

I agree

Comment thread client.go Outdated
typ, data, err := c.recvPacket(0)
if err != nil {
if err == io.EOF {
return io.ErrUnexpectedEOF

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Maybe we could do something like this here

return fmt.Errorf("server unexpectedly closed connection: %w", io.ErrUnexpectedEOF)

so errors.Is(err, io.ErrUnexpectedEOF) will work

@puellanivis

Copy link
Copy Markdown
Collaborator Author

Alright, I think we’re good now. 👍

@drakkan drakkan merged commit cf523d9 into master Jul 11, 2022
@drakkan

drakkan commented Jul 11, 2022

Copy link
Copy Markdown
Collaborator

Thanks!

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants