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

[WIP] Fix hiding of errors in SecureChannel.readChunk #551

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

Opsi
Copy link

@Opsi Opsi commented Jan 25, 2022

Related to #550

readChunk of secure_channel.go was hiding the errors of Conn.Receive, because of a wrong comparison. len(b) == 0 returns true whenever b=nil.

@magiconair
Copy link
Member

magiconair commented Jan 27, 2022

Good catch. I would suggest to fix it slightly differently since b != nil is a side effect of err == nil in this case.

if err == io.EOF || err == nil && len(b) == 0 {
    return nil, io.EOF
}

@magiconair magiconair added this to the v0.3.1 milestone Jan 27, 2022
@magiconair
Copy link
Member

Hmm, on my local branch the TestAutoReconnection/subscription_failure hangs all of a sudden.

@magiconair
Copy link
Member

Hmm, I think this is surfacing a different issue

@magiconair
Copy link
Member

have the same issue when I'm applying that patch on v0.3.0

@magiconair
Copy link
Member

I can't see it. I'll push this to v0.3.2 since this needs more investigation.

@magiconair magiconair modified the milestones: v0.3.1, v0.3.2 Jan 27, 2022
@Opsi
Copy link
Author

Opsi commented Jan 27, 2022

Sorry I totally overlooked the integration tests. The problem probably has to do with SecureChannel.dispatcher

func (s *SecureChannel) dispatcher() {
    ctx, cancel := context.WithCancel(context.Background())
    defer cancel()

    defer func() {
        close(s.disconnected)
    }()

    for {
        select {
        case <-s.closing:
            return
        default:
            resp := s.receive(ctx) // this is now returning non-io.EOF errors...
                
            if resp.Err != nil {
                select {
                case <-s.closing:
                    return
                default:
                    select {
                    case s.errCh <- resp.Err:
                    default:
                    }
                }
            }
                
            if resp.Err == io.EOF { // therefore this is NOT returning from the dispatcher loop
                return
            }
            ...

@magiconair magiconair modified the milestones: v0.3.2, v0.3.3 Mar 14, 2022
@magiconair
Copy link
Member

Hmm, when I run the test in isolation then it works every time. But not when I run the entire suite.

@magiconair magiconair modified the milestones: v0.3.3, v0.3.4 Apr 8, 2022
@magiconair
Copy link
Member

@Opsi can you rebase your branch and fix the integration tests?

@Opsi Opsi changed the title Fix hiding of errors in SecureChannel.readChunk [WIP] Fix hiding of errors in SecureChannel.readChunk May 4, 2022
@Opsi
Copy link
Author

Opsi commented May 4, 2022

Rebasing is no problem, but I don't if I can fix the integration tests.

  1. There seems to be some kind of race condition in uatest/reconnection_test.go TestAutoReconnection/subscription_failure that's (most of the time) causing an endless loop for me on the fourth run (where a "subscription_failure" is simulated)

  2. I wasn't able to look into the second error in uatest/timeout_test.go TestClientTimeoutViaContext

@magiconair magiconair modified the milestones: v0.3.4, v0.3.5 May 6, 2022
@magiconair magiconair modified the milestones: v0.3.5, v0.4 Jun 15, 2022
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