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

fix(ext,runtime): add missing custom inspections #21219

Merged
merged 15 commits into from
Nov 19, 2023

Conversation

petamoriken
Copy link
Contributor

@petamoriken petamoriken commented Nov 15, 2023

This PR includes the following fixes:

  • Change from Deno.customInspect to Deno.privateCustomInspect in internal code
  • Pass inspectOptions argument to inspect function
  • Add missing custom inspections

See comments for other minor fixes

Verified

This commit was signed with the committer’s verified signature.
KinectTheUnknown David-Joseph Xayavong

Verified

This commit was signed with the committer’s verified signature.
KinectTheUnknown David-Joseph Xayavong
fix

Verified

This commit was signed with the committer’s verified signature.
KinectTheUnknown David-Joseph Xayavong

Verified

This commit was signed with the committer’s verified signature.
KinectTheUnknown David-Joseph Xayavong

Verified

This commit was signed with the committer’s verified signature.
KinectTheUnknown David-Joseph Xayavong

Verified

This commit was signed with the committer’s verified signature.
KinectTheUnknown David-Joseph Xayavong
@petamoriken
Copy link
Contributor Author

It seems that we need to use createFilteredInspectProxy for classes with accessor properties

@petamoriken petamoriken marked this pull request as draft November 15, 2023 21:59

Verified

This commit was signed with the committer’s verified signature.
KinectTheUnknown David-Joseph Xayavong

Verified

This commit was signed with the committer’s verified signature.
KinectTheUnknown David-Joseph Xayavong
@petamoriken petamoriken marked this pull request as ready for review November 16, 2023 09:58
Comment on lines -153 to +157
length: this.length,
...ObjectFromEntries(ObjectEntries(proxy)),
})
length: this.length,
Copy link
Contributor Author

@petamoriken petamoriken Nov 16, 2023

Choose a reason for hiding this comment

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

The length property is now displayed the numeric value preferentially, in accordance with Chrome's behavior.

localStorage.setItem("length", "foo");
console.log(localStorage); // "Storage { length: 1 }"

fix

Verified

This commit was signed with the committer’s verified signature.
KinectTheUnknown David-Joseph Xayavong

Verified

This commit was signed with the committer’s verified signature.
KinectTheUnknown David-Joseph Xayavong
return `DOMException: ${this[_message]}`;
return this[_error].stack;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

DOMException's stacktrace is displayed like any other Error object

fix

Verified

This commit was signed with the committer’s verified signature.
KinectTheUnknown David-Joseph Xayavong

Verified

This commit was signed with the committer’s verified signature.
KinectTheUnknown David-Joseph Xayavong
@petamoriken
Copy link
Contributor Author

@crowlKats Please take a look!

Copy link
Member

@crowlKats crowlKats left a comment

Choose a reason for hiding this comment

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

LGTM, great to have this fixed up. Left some comments.

@petamoriken
Copy link
Contributor Author

@crowlKats fixed!

@crowlKats crowlKats merged commit c806fbd into denoland:main Nov 19, 2023
@petamoriken petamoriken deleted the fix/custom-inspect branch November 19, 2023 08:58
@mmastrac
Copy link
Contributor

This unfortunately started breaking the main build in the bench job: https://github.com/denoland/deno/actions/runs/6923517859/job/18831509208

@petamoriken
Copy link
Contributor Author

petamoriken commented Nov 20, 2023

I think it is an unrelated flaky failure, since I was able to start the Hono server in my environment without any problem.

/Users/moriken/Developer/deno/target/release/deno run --allow-all --unstable --enable-testing-features-do-not-use /Users/moriken/Developer/deno/cli/bench/http/deno_flash_hono_router.js 0.0.0.0:4553
Listening on http://localhost:4553/
Server took 38 ms to start
/Users/moriken/Developer/deno/third_party/prebuilt/mac/wrk -d 10s --latency http://127.0.0.1:4553/
Running 10s test @ http://127.0.0.1:4553/
  2 threads and 10 connections
  Thread Stats   Avg      Stdev     Max   +/- Stdev
    Latency    82.42us   30.31us   2.41ms   84.46%
    Req/Sec    54.19k     1.71k   56.17k    98.02%
  Latency Distribution
     50%   80.00us
     75%   95.00us
     90%  109.00us
     99%  145.00us
  1088425 requests in 10.10s, 158.81MB read
Requests/sec: 107769.95
Transfer/sec:     15.72MB
/home/runner/work/deno/deno/target/release/deno run --allow-all --unstable --enable-testing-features-do-not-use /home/runner/work/deno/deno/cli/bench/http/deno_flash_hono_router.js 0.0.0.0:4545
Download https://deno.land/x/hono@v2.0.9/mod.ts
thread 'main' panicked at cli/./bench/http.rs:118:29:
Failed to connect to server in time: Os *** code: 111, kind: ConnectionRefused, message: "Connection refused" ***
stack backtrace:
   0:     0x5568853a33fc - std::backtrace_rs::backtrace::libunwind::trace::h67a838aed1f4d6ec
                               at /rustc/79e9716c980570bfd1f666e3b16ac583f0168962/library/std/src/../../backtrace/src/backtrace/libunwind.rs:93:5
   1:     0x5568853a33fc - std::backtrace_rs::backtrace::trace_unsynchronized::h1d1786bb1962baf8
                               at /rustc/79e9716c980570bfd1f666e3b16ac583f0168962/library/std/src/../../backtrace/src/backtrace/mod.rs:66:5
   2:     0x5568853a33fc - std::sys_common::backtrace::_print_fmt::h5a0b1f807a002d23
                               at /rustc/79e9716c980570bfd1f666e3b16ac583f0168962/library/std/src/sys_common/backtrace.rs:67:5
   3:     0x5568853a33fc - <std::sys_common::backtrace::_print::DisplayBacktrace as core::fmt::Display>::fmt::hf84ab6ad0b91784c
                               at /rustc/79e9716c980570bfd1f666e3b16ac583f0168962/library/std/src/sys_common/backtrace.rs:44:22
   4:     0x5568853d19ec - core::fmt::rt::Argument::fmt::h28f463bd1fdabed5
                               at /rustc/79e9716c980570bfd1f666e3b16ac583f0168962/library/core/src/fmt/rt.rs:138:9
   5:     0x5568853d19ec - core::fmt::write::ha37c23b175e921b3
                               at /rustc/79e9716c980570bfd1f666e3b16ac583f0168962/library/core/src/fmt/mod.rs:1114:21
   6:     0x55688539f12e - std::io::Write::write_fmt::haa1b000741bcbbe1
                               at /rustc/79e9716c980570bfd1f666e3b16ac583f0168962/library/std/src/io/mod.rs:1[763](https://github.com/denoland/deno/actions/runs/6923517859/job/18831509208#step:49:764):15
   7:     0x5568853a31e4 - std::sys_common::backtrace::_print::h1ff1030b04dfb157
                               at /rustc/79e9716c980570bfd1f666e3b16ac583f0168962/library/std/src/sys_common/backtrace.rs:47:5
   8:     0x5568853a31e4 - std::sys_common::backtrace::print::hb982056c6f29541c
                               at /rustc/79e9716c980570bfd1f666e3b16ac583f0168962/library/std/src/sys_common/backtrace.rs:34:9
   9:     0x5568853a49f3 - std::panicking::default_hook::***closure***::h11f92f82c62fbd68
                               at /rustc/79e9716c980570bfd1f666e3b16ac583f0168962/library/std/src/panicking.rs:272:22
  10:     0x5568853a4714 - std::panicking::default_hook::hb8810fe2[767](https://github.com/denoland/deno/actions/runs/6923517859/job/18831509208#step:49:768)72c66
                               at /rustc/79e9716c980570bfd1f666e3b16ac583f0168962/library/std/src/panicking.rs:292:9
  11:     0x5568853a4fe5 - std::panicking::rust_panic_with_hook::hd2f0efd2fec86cb0
                               at /rustc/79e9716c980570bfd1f666e3b16ac583f0168962/library/std/src/panicking.rs:731:13
  12:     0x5568853a4ee1 - std::panicking::begin_panic_handler::***closure***::h3651b7fc4f61d784
                               at /rustc/79e9716c980570bfd1f666e3b16ac583f0168962/library/std/src/panicking.rs:609:13
  13:     0x5568853a3926 - std::sys_common::backtrace::__rust_end_short_backtrace::hbc468e4b98c7ae04
                               at /rustc/79e9716c980570bfd1f666e3b16ac583f0168962/library/std/src/sys_common/backtrace.rs:170:18
  14:     0x5568853a4c32 - rust_begin_unwind
                               at /rustc/79e9716c980570bfd1f666e3b16ac583f0168962/library/std/src/panicking.rs:597:5
  15:     0x5568853cf965 - core::panicking::panic_fmt::h979245e2fdb2fabd
                               at /rustc/79e9716c980570bfd1f666e3b16ac583f0168962/library/core/src/panicking.rs:72:14
  16:     0x5568853cfe43 - core::result::unwrap_failed::h8c4b86241881fbbb
                               at /rustc/79e9716c980570bfd1f666e3b16ac583f0168962/library/core/src/result.rs:1652:5
  17:     0x5568853e3a66 - deno_bench::http::run::h80b5c1c1e8be6d9d
  18:     0x5568853ee731 - deno_bench::main::***closure***::h635450f444ea0d68
  19:     0x5568853df14d - tokio::runtime::context::blocking::BlockingRegionGuard::block_on::hc[779](https://github.com/denoland/deno/actions/runs/6923517859/job/18831509208#step:49:780)a29ffba3f682
  20:     0x5568853ea975 - deno_bench::main::he61c1c0d555655ca
  21:     0x5568853dc0d0 - std::sys_common::backtrace::__rust_begin_short_backtrace::h627fff4ef02f6181
  22:     0x5568853dc3a0 - std::rt::lang_start::***closure***::hcd836c33c63a01d8
  23:     0x556885399e7b - core::ops::function::impls::<impl core::ops::function::FnOnce<A> for &F>::call_once::hf9057cfaeeb252e2
                               at /rustc/79e9716c980570bfd1f666e3b16ac583f0168962/library/core/src/ops/function.rs:284:13
  24:     0x556885399e7b - std::panicking::try::do_call::h629e203a624883e4
                               at /rustc/79e9716c9[805](https://github.com/denoland/deno/actions/runs/6923517859/job/18831509208#step:49:806)70bfd1f666e3b16ac583f0168962/library/std/src/panicking.rs:504:40
  25:     0x556885399e7b - std::panicking::try::h7b61614724d6a4f1
                               at /rustc/79e9716c980570bfd1f666e3b16ac583f0168962/library/std/src/panicking.rs:468:19
  26:     0x556885399e7b - std::panic::catch_unwind::h354ac1c0268491d8
                               at /rustc/79e9716c980570bfd1f666e3b16ac583f0168962/library/std/src/panic.rs:142:14
  27:     0x556885399e7b - std::rt::lang_start_internal::***closure***::h919fee3c5ba8f617
                               at /rustc/79e9716c980570bfd1f666e3b16ac583f0168962/library/std/src/rt.rs:148:48
  28:     0x556885399e7b - std::panicking::try::do_call::h54583f67455bff32
                               at /rustc/79e9716c980570bfd1f666e3b16ac583f0168962/library/std/src/panicking.rs:504:40
  29:     0x556885399e7b - std::panicking::try::hb0e12c4e01d39dc2
                               at /rustc/79e9716c980570bfd1f666e3b16ac583f0168962/library/std/src/panicking.rs:468:19
  30:     0x556885399e7b - std::panic::catch_unwind::h367b6339e3ca9a3b
                               at /rustc/79e9716c980570bfd1f666e3b16ac583f0168962/library/std/src/panic.rs:142:14
  31:     0x556885399e7b - std::rt::lang_start_internal::ha5ce8533eaa0fda8
                               at /rustc/79e9716c980570bfd1f666e3b16ac583f0168962/library/std/src/rt.rs:148:20
  32:     0x5568853f0ef5 - main
  33:     0x7fe1aa029d90 - <unknown>
  34:     0x7fe1aa029e40 - __libc_start_main
  35:     0x5568842b5029 - _start
  36:                0x0 - <unknown>
error: bench failed, to rerun pass `-p deno --bench deno_bench`
Error: Process completed with exit code 101.

@mmastrac
Copy link
Contributor

I think it's cargo bench that broke.

# cargo bench --bench url_ops
running 1 test
test bench_url_parse ... thread 'main' panicked at /Users/matt/.cargo/registry/src/index.crates.io-6f17d22bba15001f/deno_core-0.231.0/runtime/jsruntime.rs:806:42:
called `Result::unwrap()` on an `Err` value: Specifier "ext:deno_console/01_console.js" was not passed as an extension module and was not included in the snapshot.
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

SymbolIterator,
SymbolToStringTag,
} = primordials;
import * as webidl from "ext:deno_webidl/00_webidl.js";
import { createFilteredInspectProxy } from "ext:deno_console/01_console.js";
Copy link
Contributor

Choose a reason for hiding this comment

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

This is likely where the problem is -- we are requiring deno_console from runtime. We can temporarily work around it by adding deno_console to the url_ops benchmark but we may need move this function into runtime.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Anyway, I deleted the Worker's inspect support #21264

zifeo pushed a commit to metatypedev/deno that referenced this pull request Nov 22, 2023
crowlKats pushed a commit that referenced this pull request Nov 24, 2023
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.

None yet

3 participants