[Bugfix] Ensure writable streams are ended on PTY close #59
Conversation
daweifeng-replit
requested review from
lhchavez,
szymonkaliski,
jackyzha0 and
zhounathan
There was a problem hiding this comment.
this looks good to me, though I don't know if there aren't any intricacies downstream from this, so would love @jackyzha0 / @lhchavez to have a look too
| @@ -141,6 +142,7 @@ export class Pty { | |||
| // end instead of destroy so that the user can read the last bits of data | |||
| // and allow graceful close event to mark the fd as ended | |||
| this.#socket.end(); | |||
| this.#writable.end(); | |||
There was a problem hiding this comment.
why do we need to do this proactively instead of waiting for handleClose to be called?
There was a problem hiding this comment.
It is to prevent a race condition that there is a time window after close() is called and before handleClose() is called and during this time the underlaying socket might be closed, if it tries to write to the socket, it will fail to write. So I think it might make sense to synchronize the state of both streams? 🤔
There was a problem hiding this comment.
gotcha, that makes sense. but now we are opening ourselves for a double-close bug if the closed message is delivered after an explicit call to close(). is it totally safe to close the writable twice? if it's not, should we add one more flag to ensure that we close it at most once?
There was a problem hiding this comment.
Yes! It is idempotent. You can see that in the Writable source code below.
What is this PR?
The writable stream in PTY is never set to 'unwritable' when PTY is closed or when the child process dies.
Changes
this.#writable.end()inclose()andexit()methods of Pty class