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

Integrate src/ with core/ #1919

Closed
wants to merge 22 commits into from
Closed

Integrate src/ with core/ #1919

wants to merge 22 commits into from

Conversation

ry
Copy link
Member

@ry ry commented Mar 12, 2019

tests still not passing completely.

Depends on #1915

ry added 9 commits March 11, 2019 23:35
This is in preperation for core integration.
core: Add deno_core::Behavior trait to control Isolate

Adds basic ES module bindings to deno_core::Isolate.

clean up

respond should be private

startup_shared and startup_snapshot

Add v8_set_flags to core

core: add v8_version()

core: api adjustments

deno builds on core

Behavior should be queue instead of stack

WIP msg_ring

WIP: use msg_ring sync

core integration: Support mod_execute

reset properly

Fix module registration

core int: impl handleAsyncMsgFromRust and worker adjustments

Add reset logging

Fix msg_ring async receive bug

use split buffer

Remove reset cruft

Implement resolve callback for Cli Behavior

fix rebase
Copy link
Contributor

@afinch7 afinch7 left a comment

Choose a reason for hiding this comment

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

Figured out your problem.

src/cli.rs Outdated
state: Arc<IsolateState>,
permissions: DenoPermissions,
) -> Self {
let buffer = msg_ring::Buffer::new(1024 * 1024);
Copy link
Contributor

@afinch7 afinch7 Mar 12, 2019

Choose a reason for hiding this comment

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

This buffer is too small for some tests to complete. Might need a redesign.
Changing it to 1024 * 8196 bytes(~8MB) get us past the current failure, but errors out on a later test with:

tests/error_004_missing_module.test ... FAILED
Expected output does not match actual.
Expected output: 
Compiling [WILDCARD]tests/error_004_missing_module.ts
Uncaught NotFound: Cannot resolve module "bad-module.ts" from "[WILDCARD]/tests/error_004_missing_module.ts"
    at DenoError ([WILDCARD]/js/errors.ts:[WILDCARD])
    at maybeError ([WILDCARD]/js/errors.ts:[WILDCARD])
    at maybeThrowError ([WILDCARD]/js/errors.ts:[WILDCARD])
    at sendSync ([WILDCARD]/js/dispatch.ts:[WILDCARD])
    at fetchModuleMetaData ([WILDCARD]/js/os.ts:[WILDCARD])
    at _resolveModule ([WILDCARD]/js/compiler.ts:[WILDCARD])
    at resolveModuleNames ([WILDCARD]/js/compiler.ts:[WILDCARD])
    at compilerHost.resolveModuleNames ([WILDCARD]typescript.js:[WILDCARD])
    at resolveModuleNamesWorker ([WILDCARD]typescript.js:[WILDCARD])
    at resolveModuleNamesReusingOldState ([WILDCARD]typescript.js:[WILDCARD])

Actual output:   
Compiling file:///home/andy/Documents/projects/personal/deno/tests/error_004_missing_module.ts
Uncaught NotFound: Cannot resolve module "bad-module.ts" from "/home/andy/Documents/projects/personal/deno/tests/error_004_missing_module.ts"
    at DenoError (deno/js/errors.ts:22:5)
    at maybeError (deno/js/errors.ts:33:12)
    at maybeThrowError (deno/js/errors.ts:39:15)
    at sendSync (deno/js/dispatch.ts:87:5)
    at fetchModuleMetaData (deno/js/os.ts:81:19)
    at _resolveModule (deno/js/compiler.ts:249:38)
    at resolveModuleNames (deno/js/compiler.ts:479:35)
    at compilerHost.resolveModuleNames (deno/third_party/node_modules/typescript/lib/typescript.js:118650:138)
    at resolveModuleNamesWorker (deno/third_party/node_modules/typescript/lib/typescript.js:86767:127)
    at resolveModuleNamesReusingOldState (deno/third_party/node_modules/typescript/lib/typescript.js:87001:24)thread '
tokio-runtime-worker-0' panicked at 'called `Result::unwrap()` on an `Err` value: DenoError { repr: Simple(BadResource, "bad resource id") }', src/libcore/result.rs:997:5

This other error is unrelated.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah - thanks! I want to get the error messages when this happens to be more useful...

Copy link
Contributor

@afinch7 afinch7 Mar 12, 2019

Choose a reason for hiding this comment

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

It looks like this might become problematic. The record that failed in that test was smaller than 1048576 bytes, so it was likely caused by a chain of blocking operations. At what point are records removed from the buffer here(I assume that is what records_shift is for)? Could this garbage collection potentially be blocked by chain operations long enough to fill the buffer(I.E. lots of imports in a single file)?

Copy link
Contributor

@afinch7 afinch7 left a comment

Choose a reason for hiding this comment

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

Did some debugging, and I think I have this next one figured out. Not sure how you want't to solve it, but It looks like compile_sync needs a valid failure mode other than thread panic or js errors in workers need to handled by the worker instantiater(compile_sync in this case). I like the later better. A failure in a webworker shouldn't cause deno to exit, that's not how a browser would handle it. The instantiater should be able to decide if the worker failure is a fatal failure or not.

@@ -94,7 +93,9 @@ pub fn compile_sync(
send_future.wait().unwrap();

let recv_future = resources::worker_recv_message(compiler.rid);
let res_msg = recv_future.wait().unwrap().unwrap();
let result = recv_future.wait().unwrap();
Copy link
Contributor

@afinch7 afinch7 Mar 13, 2019

Choose a reason for hiding this comment

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

The next error I run into in tests is pretty complicated. For some reason we end up with a thread panic in the middle of our file not found error print out:

Compiling file:///home/andy/Documents/projects/personal/deno/tests/error_004_missing_module.ts
thread 'Uncaught NotFound: Cannot resolve module "bad-module.ts" from "/home/andy/Documents/projects/personal/deno/tests/error_004_missing_module.ts"
    at DenoError (deno/js/errors.ts:22:5)
    at maybeError (deno/js/errors.ts:33:12)
    at maybeThrowError (deno/js/errors.ts:39:15)
    at sendSync (deno/js/dispatch.ts:87:5)
    at fetchModuleMetaData (deno/js/os.ts:81:19)
    at _resolveModule (deno/js/compiler.ts:249:38)
    at resolveModuleNames (deno/js/compiler.ts:479:35)
    at compilerHost.resolveModuleNames (deno/third_party/node_modules/typescript/lib/typescript.js:118650:138)
    at resolveModuleNamesWorker (deno/third_party/node_modules/typescript/lib/typescript.js:86767:127)
    at resolveModuleNamesReusingOldState (deno/third_party/node_modules/typescript/lib/typescript.js:87001:24)tokio-runtime-worker-0
' panicked at 'called `Result::unwrap()` on an `Err` value: DenoError { repr: Simple(BadResource, "bad resource id") }', src/libcore/result.rs:997:5

It's not really a problem for end users, but it just makes it very difficult to test expected failures. The best way to replicate this one is with target/release/deno tests/error_004_missing_module.ts --reload.
It looks like the thread that compile_sync is running in manages to panic here before we finish our error print and exit in workers.rs.

@ry ry mentioned this pull request Mar 14, 2019
@ry
Copy link
Member Author

ry commented Mar 15, 2019

closed in favor of #1938

@ry ry closed this Mar 15, 2019
@ry ry deleted the core_improvements2 branch October 3, 2019 17:33
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.

4 participants