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

core: gracefully handle bad op id #3131

Merged
merged 3 commits into from Oct 22, 2019

Conversation

@bartlomieju
Copy link
Contributor

bartlomieju commented Oct 15, 2019

Towards #3117

@bartlomieju bartlomieju force-pushed the bartlomieju:refactor-harden_ops_api branch from f589f22 to 5964f80 Oct 15, 2019
@bartlomieju bartlomieju changed the title [WIP] refactor: harder Isolate ops API [WIP] refactor: error handling in Isolate ops API Oct 15, 2019
core/ops.rs Show resolved Hide resolved
@bartlomieju

This comment has been minimized.

Copy link
Contributor Author

bartlomieju commented Oct 20, 2019

@ry I updated bit with calling ops. Handling of malformed control buffers is still a TODO and I must say I don't know how to handle it properly...

Example:
Let's say we call "read" op with some bad control buffer, please remember that promise_id is put inside the control buffer.

Deno.core.send(1, new Uint8Array([...some junk here]))

Once we send it we get into dispatch_minimal:

pub fn minimal_op(
d: Dispatcher,
) -> impl Fn(&[u8], Option<PinnedBuf>) -> CoreOp {
move |control: &[u8], zero_copy: Option<PinnedBuf>| {
let mut record = parse_min_record(control).unwrap();
let is_sync = record.promise_id == 0;
let rid = record.arg;
let min_op = d(rid, zero_copy);

Surely parse_min_record(control).unwrap() can be handled gracefully without unwrapping, and we could construct record be hand to return error response. But here's the problem: we don't know what promise_id we should return because we couldn't get it from control buffer (because it was malformed).

That's still fine, we can create some "fake" record with error, return it to JS and then throw error there, but there's another problem...

Remember that promise_id we're missing? That means that underlying promise (still in promise map) would still pend resolution - but we don't know what was the promise id so effectively we can't remove promise from the map and reject it.

Exactly the same scenario applies to JSON dispatch.

EDIT 2: Simplest approach would be to move promise_id to arguments for Deno.core.send but I can remember you were reluctant to do that some time ago.

EDIT: While we're at it, we might tackle the problem with minimal dispatch not returning error properly - how about returning 4 fields in record instead of 3? Alternatively we could encode errors using negative numbers.

@bartlomieju bartlomieju force-pushed the bartlomieju:refactor-harden_ops_api branch from 2f10fe0 to 3cd2f5d Oct 20, 2019
@bartlomieju bartlomieju changed the title [WIP] refactor: error handling in Isolate ops API core: gracefully handle bad op id Oct 22, 2019
@bartlomieju

This comment has been minimized.

Copy link
Contributor Author

bartlomieju commented Oct 22, 2019

@ry this PR should be landable

@ry
ry approved these changes Oct 22, 2019
Copy link
Collaborator

ry left a comment

LGTM - I'm glad to be getting rid of all these runtime panics

@ry ry merged commit 6257b40 into denoland:master Oct 22, 2019
10 checks passed
10 checks passed
test macOS-10.14
Details
test_std macOS-10.14
Details
test windows-2016
Details
test_std windows-2016
Details
test ubuntu-16.04
Details
test_debug ubuntu-16.04
Details
test_std ubuntu-16.04
Details
bench ubuntu-16.04
Details
lint ubuntu-16.04
Details
license/cla Contributor License Agreement is signed.
Details
@bartlomieju bartlomieju deleted the bartlomieju:refactor-harden_ops_api branch Oct 22, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
2 participants
You can’t perform that action at this time.