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.

Sign up for GitHub

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

Jump to bottom

[Bug]: hijacked connection returned by ContainerExecAttach is not closed #1357

Open
AlexanderYastrebov opened this issue Jul 10, 2023 · 1 comment
Open

[Bug]: hijacked connection returned by ContainerExecAttach is not closed #1357

AlexanderYastrebov opened this issue Jul 10, 2023 · 1 comment
Labels
bug An issue with the library

Comments

@AlexanderYastrebov
Copy link
Contributor

AlexanderYastrebov commented Jul 10, 2023 •
edited

Testcontainers version

v0.21.0

Using the latest Testcontainers version?

Yes

Host OS

Linux

Host arch

x86

Go version

1.20

Docker version

-

Docker info

-

What happened?

Here

testcontainers-go/docker.go

Lines 485 to 492 in f5a4a54

hijack, err := cli.ContainerExecAttach(ctx, response.ID, types.ExecStartCheck{})
if err != nil {
return 0, nil, err
}
opt := &tcexec.ProcessOptions{
Reader: hijack.Reader,
}

cli.ContainerExecAttach returns hijacked connections which should be closed by the caller:

It's up to the called to close the hijacked connection by calling types.HijackedResponse.Close.

Unfortunately Exec returns io.Reader

testcontainers-go/container.go

Line 51 in f5a4a54

Exec(ctx context.Context, cmd []string, options ...tcexec.ProcessOption) (int, io.Reader, error)

I think it should return io.ReadCloser that closes hijacked connection on Close()

See related #321

Relevant log output

No response

Additional information

No response
@AlexanderYastrebov AlexanderYastrebov added the bug An issue with the library label Jul 10, 2023
@AlexanderYastrebov AlexanderYastrebov mentioned this issue Jul 10, 2023
Closed
AlexanderYastrebov added a commit to AlexanderYastrebov/testcontainers-go that referenced this issue Jul 10, 2023
@AlexanderYastrebov
all: fix goroutine leaks
e10a9d7 
Fixes various goroutine leaks by moving around and adding missing Close calls.

Leaks were detected by github.com/AlexanderYastrebov/noleak by adding

```go
// go get github.com/AlexanderYastrebov/noleak@latest
// cat main_test.go
package testcontainers

import (
        "os"
        "testing"

        "github.com/AlexanderYastrebov/noleak"
)

func TestMain(m *testing.M) {
        os.Exit(noleak.CheckMain(m))
}
```

Not all leaks could be fixed e.g. due to testcontainers#1357 therefore this change
does not add leak detector, only fixes.

Updates testcontainers#321
@AlexanderYastrebov AlexanderYastrebov mentioned this issue Jul 10, 2023
Merged
AlexanderYastrebov added a commit to AlexanderYastrebov/testcontainers-go that referenced this issue Jul 12, 2023
@AlexanderYastrebov
all: fix goroutine leaks
3febcdc 
Fixes various goroutine leaks by moving around and adding missing Close calls.

Leaks were detected by github.com/AlexanderYastrebov/noleak by adding

```go
// go get github.com/AlexanderYastrebov/noleak@latest
// cat main_test.go
package testcontainers

import (
        "os"
        "testing"

        "github.com/AlexanderYastrebov/noleak"
)

func TestMain(m *testing.M) {
        os.Exit(noleak.CheckMain(m))
}
```

Not all leaks could be fixed e.g. due to testcontainers#1357 therefore this change
does not add leak detector, only fixes.

Updates testcontainers#321
@mdelapenya
Copy link
Collaborator

Hi @AlexanderYastrebov thanks for reporting this. I now understand the issue with the Exec function not returning a ReadCloser.

I think we can update the signature as a breaking change in an upcoming minor release if needed, although I'm planning to start working on a v1 branch for an eventual v1.0.0 release of tc-go.

AlexanderYastrebov added a commit to AlexanderYastrebov/testcontainers-go that referenced this issue Aug 5, 2023
@AlexanderYastrebov
all: fix goroutine leaks
e5c9f9f 
Fixes various goroutine leaks by moving around and adding missing Close calls.

Leaks were detected by github.com/AlexanderYastrebov/noleak by adding

```go
// go get github.com/AlexanderYastrebov/noleak@latest
// cat main_test.go
package testcontainers

import (
        "os"
        "testing"

        "github.com/AlexanderYastrebov/noleak"
)

func TestMain(m *testing.M) {
        os.Exit(noleak.CheckMain(m))
}
```

Not all leaks could be fixed e.g. due to testcontainers#1357 therefore this change
does not add leak detector, only fixes.

Updates testcontainers#321
AlexanderYastrebov added a commit to AlexanderYastrebov/testcontainers-go that referenced this issue Aug 5, 2023
@AlexanderYastrebov
all: fix goroutine leaks
74f21a9 
Fixes various goroutine leaks by moving around and adding missing Close calls.

Leaks were detected by github.com/AlexanderYastrebov/noleak by adding

```go
// go get github.com/AlexanderYastrebov/noleak@latest
// cat main_test.go
package testcontainers

import (
        "os"
        "testing"

        "github.com/AlexanderYastrebov/noleak"
)

func TestMain(m *testing.M) {
        os.Exit(noleak.CheckMain(m))
}
```

Not all leaks could be fixed e.g. due to testcontainers#1357 therefore this change
does not add leak detector, only fixes.

Updates testcontainers#321
AlexanderYastrebov added a commit to AlexanderYastrebov/testcontainers-go that referenced this issue Aug 6, 2023
@AlexanderYastrebov
all: fix goroutine leaks
b5b70e3 
Fixes various goroutine leaks by moving around and adding missing Close calls.

Leaks were detected by github.com/AlexanderYastrebov/noleak by adding

```go
// go get github.com/AlexanderYastrebov/noleak@latest
// cat main_test.go
package testcontainers

import (
        "os"
        "testing"

        "github.com/AlexanderYastrebov/noleak"
)

func TestMain(m *testing.M) {
        os.Exit(noleak.CheckMain(m))
}
```

Not all leaks could be fixed e.g. due to testcontainers#1357 therefore this change
does not add leak detector, only fixes.

Updates testcontainers#321
AlexanderYastrebov added a commit to AlexanderYastrebov/testcontainers-go that referenced this issue Aug 6, 2023
@AlexanderYastrebov
all: fix goroutine leaks
2b055df 
Fixes various goroutine leaks by moving around and adding missing Close calls.

Leaks were detected by github.com/AlexanderYastrebov/noleak by adding

```go
// go get github.com/AlexanderYastrebov/noleak@latest
// cat main_test.go
package testcontainers

import (
        "os"
        "testing"

        "github.com/AlexanderYastrebov/noleak"
)

func TestMain(m *testing.M) {
        os.Exit(noleak.CheckMain(m))
}
```

Not all leaks could be fixed e.g. due to testcontainers#1357 therefore this change
does not add leak detector, only fixes.

Updates testcontainers#321
mdelapenya pushed a commit that referenced this issue Aug 9, 2023
@AlexanderYastrebov
all: fix goroutine leaks (#1358)
7b4cf84 
Fixes various goroutine leaks by moving around and adding missing Close calls.

Leaks were detected by github.com/AlexanderYastrebov/noleak by adding

```go
// go get github.com/AlexanderYastrebov/noleak@latest
// cat main_test.go
package testcontainers

import (
        "os"
        "testing"

        "github.com/AlexanderYastrebov/noleak"
)

func TestMain(m *testing.M) {
        os.Exit(noleak.CheckMain(m))
}
```

Not all leaks could be fixed e.g. due to #1357 therefore this change
does not add leak detector, only fixes.

Updates #321
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
bug An issue with the library
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
2 participants
@AlexanderYastrebov @mdelapenya