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(core): avoid op_state.borrow_mut() for OpsTracker #12525

Merged
merged 5 commits into from
Oct 24, 2021

Conversation

AaronO
Copy link
Contributor

@AaronO AaronO commented Oct 24, 2021

By allowing interior mutability in OpsTracker (owning a RefCell<Vec> instead of just a Vec)

Fixes #12453

High level test

rm -rf ~/Library/Caches/deno/deps/https; deno --version; deno eval --unstable "console.log(await Deno.emit('https://deno.land/std/datetime/mod.ts'));"
deno 1.15.2 (release, aarch64-apple-darwin)
v8 9.5.172.19
typescript 4.4.2
Download https://deno.land/std/datetime/mod.ts
thread 'main' panicked at 'already borrowed: BorrowMutError', core/bindings.rs:423:22
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

~/git/deno fix/core-tracker-borrowmut-errrm -rf ~/Library/Caches/deno/deps/https; ./target/debug/deno eval --unstable "console.log(await Deno.emit('https://deno.land/std/datetime/mod.ts'));"
Download https://deno.land/std/datetime/mod.ts
Warning Implicitly using latest version (0.112.0) for https://deno.land/std/datetime/mod.ts
Download https://deno.land/std@0.112.0/datetime/mod.ts
Download https://deno.land/std@0.112.0/datetime/formatter.ts
Download https://deno.land/std@0.112.0/datetime/tokenizer.ts
{
  diagnostics: [],
  files: {
    "https://deno.land/std@0.112.0/datetime/mod.ts.js.map": '{"version":3,"file":"","sourceRoot":"","sources":["https://deno.land/std@0.112.0/datetime/mod.ts"],"...',
    "https://deno.land/std@0.112.0/datetime/formatter.ts.js": 'import { Tokenizer, } from "./tokenizer.ts";\nfunction digits(value, count = 2) {\n    return String(v...',
    "https://deno.land/std@0.112.0/datetime/formatter.ts.js.map": '{"version":3,"file":"","sourceRoot":"","sources":["https://deno.land/std@0.112.0/datetime/formatter....',
    "https://deno.land/std@0.112.0/datetime/tokenizer.ts.js": "export class Tokenizer {\n    rules;\n    constructor(rules = []) {\n        this.rules = rules;\n    }\n...",
    "https://deno.land/std@0.112.0/datetime/tokenizer.ts.js.map": '{"version":3,"file":"","sourceRoot":"","sources":["https://deno.land/std@0.112.0/datetime/tokenizer....',
    "https://deno.land/std@0.112.0/datetime/mod.ts.js": 'import { DateTimeFormatter } from "./formatter.ts";\nexport const SECOND = 1e3;\nexport const MINUTE =...'
  },
  ignoredOptions: null,
  stats: []
}

Notes

  • Missing a unit test of an async-op borrowing OpState across an await
  • We'll probably want to rethink OpState, since variants of Deno.emit crashes with BorrowMutError #12453 could happen if OpState is mutably borrowed across an await (took a quick look and didn't see any instances of it in the codebase)

@bartlomieju
Copy link
Member

bartlomieju commented Oct 24, 2021

We'll probably want to rethink OpState, since variants of Deno.emit crashes with BorrowMutError #12453 could happen if OpState is mutably borrowed across an await (took a quick look and didn't see any instances of it in the codebase)

The whole idea is that you shouldn't hold borrows to RefCell across await point, there's even a clippy lint that checks that. I believe this is fine and we don't need to change it.

@AaronO AaronO merged commit 439a291 into denoland:main Oct 24, 2021
@AaronO AaronO deleted the fix/core-tracker-borrowmut-err branch October 24, 2021 17:30
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.

Deno.emit crashes with BorrowMutError
2 participants