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.

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

Jump to bottom

fix(repl): prevent panic when deleting globalThis.closed property #24014

Merged
merged 5 commits into from
Jun 18, 2024
Merged

fix(repl): prevent panic when deleting globalThis.closed property #24014

Changes from 2 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
aa165fd
fix(repl): prevent panic when deleting globalThis.closed property
safaa-mojahed May 28, 2024
53b8dee
Merge branch 'main' into fix-panic-delete-closed-property
safaa-mojahed May 28, 2024
0eadf36
fix(repl): prevent panic when deleting globalThis.closed property by …
safaa-mojahed May 29, 2024
1bb6505
Merge branch 'main' into fix-panic-delete-closed-property
safaa-mojahed May 29, 2024
eee5378
Merge branch 'main' into fix-panic-delete-closed-property
dsherret Jun 17, 2024
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
4 changes: 3 additions & 1 deletion cli/tools/repl/session.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -99,6 +99,7 @@ Object.defineProperty(globalThis, "{0}", {{
lastThrownError: undefined,
inspectArgs: Deno[Deno.internal].inspectArgs,
noColor: Deno.noColor,
closed: this.closed
}},
}});
Object.defineProperty(globalThis, "_", {{
Expand Down Expand Up @@ -299,8 +300,9 @@ impl ReplSession {
}

pub async fn closing(&mut self) -> Result<bool, AnyError> {
let expression = format!(r#"{}.closed"#, *REPL_INTERNALS_NAME);
let closed = self
.evaluate_expression("(this.closed)")
.evaluate_expression(&expression)
Copy link
Member

Choose a reason for hiding this comment

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

What sets this value now?

Copy link
Member

Choose a reason for hiding this comment

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

@dsherret it's still set by calling close().

Copy link
Member

Choose a reason for hiding this comment

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

Won't that only set on globalThis?

Copy link
Member

Choose a reason for hiding this comment

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

It would, but if you delete it then calling close() will just throw an error saying that it's not defined. Which is okay. But since we check value of globalThis.closed in the REPL look it causes a panic when deleting globalThis.

Copy link
Member

@dsherret dsherret May 28, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Yeah, but this code always returns false now because it's only set on startup. It seems the repl::close_command test is failing now because this is never set to true.

Copy link
Contributor Author

@safaa-mojahed safaa-mojahed May 28, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I thought this.closed is always evaluated on every access since it is a getter, but from the test I can see that it is only evaluated once. I think I will change this line "closed: this.closed" to getter so it should be evaluated every access. But when the closed property is deleted should I set a default value to return or what??

@dsherret

Copy link
Member

Choose a reason for hiding this comment

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

I guess another option that we can try is to save the reference to globalThis in the "internals" object and then access the getter from this saved reference.

Copy link
Contributor Author

@safaa-mojahed safaa-mojahed May 29, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

@bartlomieju Using the getter won't prevent the panic issue. Alternatively, should I return a default value if this.closed is returned undefined?

image

.await?
.result
.value
Expand Down
Loading