Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
6 changes: 6 additions & 0 deletions src/Porta.Pty/Linux/PtyConnection.cs
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,12 @@ protected override bool Resize(int controller, int cols, int rows)
return pty_resize(controller, (ushort)rows, (ushort)cols) != -1;
}

/// <inheritdoc/>
protected override bool Close(int controller)
{
return pty_close(controller) != -1;
}

/// <inheritdoc/>
protected override bool WaitPid(int pid, ref int status)
{
Expand Down
6 changes: 6 additions & 0 deletions src/Porta.Pty/Mac/PtyConnection.cs
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,12 @@ protected override bool Resize(int controller, int cols, int rows)
return pty_resize(controller, (ushort)rows, (ushort)cols) != -1;
}

/// <inheritdoc/>
protected override bool Close(int controller)
{
return pty_close(controller) != -1;
}

/// <inheritdoc/>
protected override bool WaitPid(int pid, ref int status)
{
Expand Down
36 changes: 36 additions & 0 deletions src/Porta.Pty/Unix/PtyConnection.cs
Original file line number Diff line number Diff line change
Expand Up @@ -77,6 +77,22 @@ public void Dispose()

// Try to kill the process, but don't throw if it already exited
this.TryKill();

// ...and then CLOSE the controller fd, which nothing used to do.
//
// PtyStream wraps the fd with ownsHandle: false -- deliberately, because ReaderStream and
// WriterStream are two streams over the SAME fd and letting either own it would make
// disposing both a double close. The consequence was that disposing them closed neither,
// and pty_close, which exists in the shim and in both platforms' NativeMethods, had no
// callers anywhere in the library. Every pseudoterminal ever opened leaked its fd for the
// life of the process.
//
// It surfaces as pty_spawn failing with ENXIO ("no pty devices available") after enough
// terminals have come and gone -- which reads as a system limit rather than as a leak, and
// reads that way most convincingly in exactly the long-lived process that leaks the most.
// Found because moving this suite into a single MTP process ran four 24-spawn tests
// back to back and the third started failing; each test alone had always been fine.
this.TryClose();
}

/// <inheritdoc/>
Expand Down Expand Up @@ -124,6 +140,13 @@ public bool WaitForExit(int milliseconds)
/// <returns>True if the function succeeded in killing the process, false otherwise.</returns>
protected abstract bool Kill(int controller);

/// <summary>
/// OS-specific implementation of closing the pty controller fd.
/// </summary>
/// <param name="controller">The fd of the pty controller.</param>
/// <returns>True if the fd was closed, false otherwise.</returns>
protected abstract bool Close(int controller);

/// <summary>
/// OS-specific implementation of waiting on the given process id.
/// </summary>
Expand All @@ -147,6 +170,19 @@ private void TryKill()
}
}

/// <summary>Closes the controller fd without throwing; it may already be gone.</summary>
private void TryClose()
{
try
{
this.Close(this.controller);

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

This maybe should be fixed

}
catch
{
// Ignore errors during cleanup.
}
}

private void ChildWatcherThreadProc()
{
Debug.WriteLine($"Waiting on {this.pid}");
Expand Down
Loading