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.

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

[WIP] feat(unstable): add Deno.openPty and friends #14914

Closed
wants to merge 35 commits into from
Closed

[WIP] feat(unstable): add Deno.openPty and friends #14914

wants to merge 35 commits into from

Conversation

imjamesb
Copy link
Contributor

Original PR: #14535, github repo got deleted so I had to reopen.

imjamesb and others added 26 commits June 19, 2022 16:38
* feat(serde_v8): add serde_v8::Global (#14761)

* chore: use rust 1.61.0 (#14911)

* refactor: make MainWorker::evaluate_module public (#14892)

* fix(ext/fetch): add `accept-language` default header to fetch (#14882)

Co-authored-by: Divy Srivastava <[email protected]>
Co-authored-by: Aaron O'Mullan <[email protected]>
Co-authored-by: Mathias Lafeldt <[email protected]>
Co-authored-by: Mark Ladyshau <[email protected]>
Signed-off-by: James Bradlee <[email protected]>
Signed-off-by: James Bradlee <[email protected]>
Signed-off-by: James Bradlee <[email protected]>
Signed-off-by: James Bradlee <[email protected]>
Signed-off-by: James Bradlee <[email protected]>
* feat(serde_v8): add serde_v8::Global (#14761)

* chore: use rust 1.61.0 (#14911)

* refactor: make MainWorker::evaluate_module public (#14892)

* fix(ext/fetch): add `accept-language` default header to fetch (#14882)

Co-authored-by: Divy Srivastava <[email protected]>
Co-authored-by: Aaron O'Mullan <[email protected]>
Co-authored-by: Mathias Lafeldt <[email protected]>
Co-authored-by: Mark Ladyshau <[email protected]>
Signed-off-by: James Bradlee <[email protected]>
Signed-off-by: James Bradlee <[email protected]>
Comment on lines 256 to 260
export interface Pty {
rid: number;
readable: ReadableStream<Uint8Array>;
writable: WritableStream<Uint8Array>;
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Add a close() method like other resources have

Suggested change
export interface Pty {
rid: number;
readable: ReadableStream<Uint8Array>;
writable: WritableStream<Uint8Array>;
}
export class Pty {
readonly rid: number;
readonly readable: ReadableStream<Uint8Array>;
readonly writable: WritableStream<Uint8Array>;
close(): void;
}

Copy link
Member

Choose a reason for hiding this comment

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

@nayeemrmn not all resources have that, ie the new subprocess API does not have a close method. also on that note, the rid should not be exposed either.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Spawn has alternate ways of closing / being waited on, things that are closed primarily by Deno.close(res.rid) should have a wrapper method.

Hiding rids (even if they're not used for normal API usage) might be a problem for metrics and resource leakage tracking, but I'm not sure if that's practically a problem especially with our better sanitizer messages. Sure, I agree with hiding rid in a weak map for now.

Copy link
Member

@crowlKats crowlKats Jun 21, 2022

Choose a reason for hiding this comment

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

I agree with hiding rid in a weak map for now.

instead of a weakmap, it should be a symbol (as we do with web apis), for consistency.

overall, from what i remember discussing a few months back, we would want to move away from allowing manual resource management (so closing a resource and other usages of resource ids)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@nayeemrmn I suppose that this method will use core.tryClose() under the hood?

* console.log("%sx%s", size.columns, size.rows);
* `
* ],
* tty: pty.rid,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Accept the Pty object directly, unwrap the rid when calling the op

Suggested change
* tty: pty.rid,
* tty: pty,

Copy link
Collaborator

@nayeemrmn nayeemrmn left a comment

Choose a reason for hiding this comment

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

Deno's later APIs, while not hiding the rid, are designed to be usable without referencing them. See Deno.HttpClient:

const client = Deno.createHttpClient({ caCerts: [ caCert ] });
const req = await fetch("https://myserver.com", { client });
client.close();

@imjamesb
Copy link
Contributor Author

So I think the PR should be complete Unix wise, maybe some tests is needed first. I'll start getting work done on the Windows features ASAP.

@imjamesb
Copy link
Contributor Author

My PC's network card is broken. Can't work on the Windows implementation. I'll go fetch my work laptop in a couple of days to continue working on this.

@imjamesb
Copy link
Contributor Author

I got my network card working, just doing some research on conpty, just need to learn the infrastructure around it for Windows.

@imjamesb
Copy link
Contributor Author

@imjamesb
Copy link
Contributor Author

Another thing that should be taken into consideration if PTY's are moved into spawn API is that consoleSize() should get the PTY console size for the attached process, which will cause issues if implemented incorrectly on Linux, but will work ok on Windows. And then consoleResize which should resize the PTY, so in this case the operation should be applied to the PTY and not the process. I think that some changes are required in jsland to support those new features.

@imjamesb
Copy link
Contributor Author

imjamesb commented Aug 23, 2022

Just because I want this so bad I will take another shot at implementing this into the spawn API and also introduce a new tty function, Deno.consoleResize().

edit: Nevermind #15641

@imjamesb imjamesb closed this Aug 27, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants