Skip to content

fix(pty): close output writer before reader on Windows to unblock close #8299

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

Merged
merged 6 commits into from
Jul 5, 2023
Merged

fix(pty): close output writer before reader on Windows to unblock close #8299

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
5770111
fix(pty): consume final console frame on Windows to unblock close
mafredri Jul 3, 2023
c3f472b
test
mafredri Jul 3, 2023
2bd817d
debug
mafredri Jul 3, 2023
53fb4c8
improve ptytest logs
mafredri Jul 3, 2023
068ed51
cleanup comment
mafredri Jul 3, 2023
3762cb8
fix wrong assumptions
mafredri Jul 5, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
81 changes: 56 additions & 25 deletions pty/pty_windows.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -65,10 +65,13 @@ func newPty(opt ...Option) (*ptyWindows, error) {
0,
uintptr(unsafe.Pointer(&pty.console)),
)
if int32(ret) < 0 {
// CreatePseudoConsole returns S_OK on success, as per:
// https://learn.microsoft.com/en-us/windows/console/createpseudoconsole
if windows.Handle(ret) != windows.S_OK {
_ = pty.Close()
return nil, xerrors.Errorf("create pseudo console (%d): %w", int32(ret), err)
}

return pty, nil
}

Expand Down Expand Up @@ -134,39 +137,69 @@ func (p *ptyWindows) Resize(height uint16, width uint16) error {
Y: int16(height),
X: int16(width),
})))))
if ret != 0 {
if windows.Handle(ret) != windows.S_OK {
return err
}
return nil
}

func (p *ptyWindows) Close() error {
p.closeMutex.Lock()
defer p.closeMutex.Unlock()
if p.closed {
return nil
}
p.closed = true

// closeConsoleNoLock closes the console handle, and sets it to
// windows.InvalidHandle. It must be called with p.closeMutex held.
func (p *ptyWindows) closeConsoleNoLock() error {
// if we are running a command in the PTY, the corresponding *windowsProcess
// may have already closed the PseudoConsole when the command exited, so that
// output reads can get to EOF. In that case, we don't need to close it
// again here.
if p.console != windows.InvalidHandle {
//
// Because ClosePseudoConsole has no return value, we could ignore the
// error here, but we limit this either S_OK or S_FALSE as a safety-net
// in case other values could be returned.
// https://docs.microsoft.com/en-us/windows/console/closepseudoconsole
ret, _, err := procClosePseudoConsole.Call(uintptr(p.console))
if ret < 0 {
return xerrors.Errorf("close pseudo console: %w", err)
if ret := windows.Handle(ret); ret != windows.S_OK && ret != windows.S_FALSE {
Copy link
Member Author

Choose a reason for hiding this comment

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

Review: I contemplated ignoring ret/err here entirely, however, in my testing it seems this function usually returns S_FALSE (1). Since I can't find documentation for this behavior, we allow S_OK too. Given positive error values, limiting this to negative doesn't make sense to me: https://learn.microsoft.com/en-us/windows/win32/seccrypto/common-hresult-values

Copy link
Contributor

Choose a reason for hiding this comment

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

Values like 0x80004001 are negative when interpreted as a signed 32 bit integer because they are encoded in two's compliment

Copy link
Contributor

Choose a reason for hiding this comment

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

S_FALSE is a "success" code, so I think it's still fine to check ret<0 for in general.

In this specific case, Windows docs say that there is no return value, so my guess is that the Windows syscall libraries, or perhaps the Windows kernel itself is writing S_FALSE for this procedure to indicate that it's not an explicit "OK" because the call has no return value.

We could just drop the whole error handling block here and cite the docs that say this procedure doesn't return any values.

Copy link
Member Author

Choose a reason for hiding this comment

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

Values like 0x80004001 are negative when interpreted as a signed 32 bit integer because they are encoded in two's compliment

True, however the return value from Call is an uintptr and this ret wasn't being cast to 32-bit. As such the ret < 0 was nonsensical. I don't know enough about Windows syscall internals to know if we can always cast it, that would assume a syscall always returns HRESULT (maybe it does).

In this specific case, Windows docs say that there is no return value, so my guess is that the Windows syscall libraries, or perhaps the Windows kernel itself is writing S_FALSE for this procedure to indicate that it's not an explicit "OK" because the call has no return value.

Sounds plausible, this value is at least not coming from Go and Windows does define success as a positive 32-bit integer.

We could just drop the whole error handling block here and cite the docs that say this procedure doesn't return any values.

I would still like to keep it, just in case. Unless we can confirm that this syscall can't fail in some way or form.

Copy link
Member Author

Choose a reason for hiding this comment

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

Changed this to int32(ret) < 0 now, to be more generic and self-documenting via a new func winerrorFailed(r1 uintptr) bool function.

return xerrors.Errorf("close pseudo console (%d): %w", ret, err)
}
p.console = windows.InvalidHandle
}

// We always have these files
_ = p.outputRead.Close()
_ = p.inputWrite.Close()
// These get closed & unset if we Start() a new process.
return nil
}

func (p *ptyWindows) Close() error {
p.closeMutex.Lock()
defer p.closeMutex.Unlock()
if p.closed {
return nil
}

err := p.closeConsoleNoLock()
if err != nil {
return err
}

// Only set closed after the console has been successfully closed.
p.closed = true

// Tear down the pipes to unblock readers/writers. Note that outputWrite and
// inputRead are unset when we Start() a new process.
//
// The console is closed so there will be no more writes, we need to do this
// here to ensure the discard below is unblocked.
if p.outputWrite != nil {
_ = p.outputWrite.Close()
}
// Unless we drain outputRead here, calling Close may wait indefinitely:
// https://learn.microsoft.com/en-us/windows/console/closepseudoconsole
//
// Note that this may result in a small amount of unprocessed output
// being lost, but the alternative is to wrap the reader or to handle
// this by all consumers.
_, _ = io.Copy(io.Discard, p.outputRead)

_ = p.outputRead.Close()
_ = p.inputWrite.Close()

if p.inputRead != nil {
_ = p.inputRead.Close()
}
Expand All @@ -184,15 +217,13 @@ func (p *windowsProcess) waitInternal() {
// c.f. https://devblogs.microsoft.com/commandline/windows-command-line-introducing-the-windows-pseudo-console-conpty/
p.pw.closeMutex.Lock()
defer p.pw.closeMutex.Unlock()
if p.pw.console != windows.InvalidHandle {
ret, _, err := procClosePseudoConsole.Call(uintptr(p.pw.console))
if ret < 0 && p.cmdErr == nil {
// if we already have an error from the command, prefer that error
// but if the command succeeded and closing the PseudoConsole fails
// then record that error so that we have a chance to see it
p.cmdErr = err
}
p.pw.console = windows.InvalidHandle

err := p.pw.closeConsoleNoLock()
// if we already have an error from the command, prefer that error
// but if the command succeeded and closing the PseudoConsole fails
// then record that error so that we have a chance to see it
if err != nil && p.cmdErr == nil {
p.cmdErr = err
}
}()

Expand Down
16 changes: 14 additions & 2 deletions pty/ptytest/ptytest.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -354,7 +354,13 @@ type PTY struct {
func (p *PTY) Close() error {
p.t.Helper()
pErr := p.PTY.Close()
eErr := p.outExpecter.close("close")
if pErr != nil {
p.logf("PTY Close PTY: %v", pErr)
}
eErr := p.outExpecter.close("PTY close")
if eErr != nil {
p.logf("PTY Close expecter: %v", eErr)
}
if pErr != nil {
return pErr
}
Expand Down Expand Up @@ -398,7 +404,13 @@ type PTYCmd struct {
func (p *PTYCmd) Close() error {
p.t.Helper()
pErr := p.PTYCmd.Close()
eErr := p.outExpecter.close("close")
if pErr != nil {
p.logf("PTYCmd Close PTYCmd: %v", pErr)
}
eErr := p.outExpecter.close("PTYCmd close")
if eErr != nil {
p.logf("PTYCmd Close expecter: %v", eErr)
}
if pErr != nil {
return pErr
}
Expand Down