Skip to content

Commit

Permalink
fix: Leaking yamux session after HTTP handler is closed
Browse files Browse the repository at this point in the history
Closes #317. The httptest server cancels the context after the connection
is closed, but if a connection takes a long time to close, the request
would never end. This applies a context to the entire listener that cancels
on test cleanup.

After discussion with @bryphe-coder, reducing the parallel limit on
Windows is likely to reduce failures as well.
  • Loading branch information
kylecarbs committed Feb 18, 2022
1 parent 5ef59e7 commit 6126814
Show file tree
Hide file tree
Showing 4 changed files with 27 additions and 6 deletions.
9 changes: 6 additions & 3 deletions .github/workflows/coder.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -150,9 +150,12 @@ jobs:
terraform_wrapper: false

- name: Test with Mock Database
env:
GOCOUNT: ${{ runner.os == 'Windows' && 3 || 5 }}
GOMAXPROCS: ${{ runner.os == 'Windows' && 1 || 2 }}
run: gotestsum --junitfile="gotests.xml" --packages="./..." --
-covermode=atomic -coverprofile="gotests.coverage"
-timeout=3m -count=5 -race -short -parallel=2
-timeout=3m -count=$GOCOUNT -race -short -failfast

- name: Upload DataDog Trace
if: (success() || failure()) && github.actor != 'dependabot[bot]'
Expand All @@ -166,10 +169,10 @@ jobs:
if: runner.os == 'Linux'
run: DB=true gotestsum --junitfile="gotests.xml" --packages="./..." --
-covermode=atomic -coverprofile="gotests.coverage" -timeout=3m
-count=1 -race -parallel=2
-count=1 -race -parallel=2 -failfast

- name: Upload DataDog Trace
if: (success() || failure()) && github.actor != 'dependabot[bot]'
if: (success() || failure()) && github.actor != 'dependabot[bot]' && runner.os == 'Linux'
env:
DATADOG_API_KEY: ${{ secrets.DATADOG_API_KEY }}
DD_DATABASE: postgresql
Expand Down
9 changes: 8 additions & 1 deletion coderd/coderdtest/coderdtest.go
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import (
"context"
"database/sql"
"io"
"net"
"net/http/httptest"
"net/url"
"os"
Expand Down Expand Up @@ -59,7 +60,13 @@ func New(t *testing.T) *codersdk.Client {
Database: db,
Pubsub: pubsub,
})
srv := httptest.NewServer(handler)
srv := httptest.NewUnstartedServer(handler)
srv.Config.BaseContext = func(_ net.Listener) context.Context {
ctx, cancelFunc := context.WithCancel(context.Background())
t.Cleanup(cancelFunc)
return ctx
}
srv.Start()
serverURL, err := url.Parse(srv.URL)
require.NoError(t, err)
t.Cleanup(srv.Close)
Expand Down
7 changes: 7 additions & 0 deletions pty/pty_other.go
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ package pty
import (
"io"
"os"
"sync"

"github.com/creack/pty"
)
Expand All @@ -23,6 +24,7 @@ func newPty() (PTY, error) {
}

type otherPty struct {
mutex sync.Mutex
pty, tty *os.File
}

Expand All @@ -41,13 +43,18 @@ func (p *otherPty) Output() io.ReadWriter {
}

func (p *otherPty) Resize(cols uint16, rows uint16) error {
p.mutex.Lock()
defer p.mutex.Unlock()
return pty.Setsize(p.tty, &pty.Winsize{
Rows: rows,
Cols: cols,
})
}

func (p *otherPty) Close() error {
p.mutex.Lock()
defer p.mutex.Unlock()

err := p.pty.Close()
if err != nil {
return err
Expand Down
8 changes: 6 additions & 2 deletions pty/start_other.go
Original file line number Diff line number Diff line change
Expand Up @@ -8,13 +8,17 @@ import (
"syscall"

"github.com/creack/pty"
"golang.org/x/xerrors"
)

func startPty(cmd *exec.Cmd) (PTY, error) {
ptty, tty, err := pty.Open()
if err != nil {
return nil, err
return nil, xerrors.Errorf("open: %w", err)
}
defer func() {
_ = tty.Close()
}()
cmd.SysProcAttr = &syscall.SysProcAttr{
Setsid: true,
Setctty: true,
Expand All @@ -25,7 +29,7 @@ func startPty(cmd *exec.Cmd) (PTY, error) {
err = cmd.Start()
if err != nil {
_ = ptty.Close()
return nil, err
return nil, xerrors.Errorf("start: %w", err)
}
return &otherPty{
pty: ptty,
Expand Down

0 comments on commit 6126814

Please sign in to comment.