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

Add bool checkedMode optional, named arg to spawn/spawnUri isolate functions #21791

Closed
nex3 opened this issue Dec 4, 2014 · 21 comments
Closed
Assignees
Labels
area-core-library SDK core library issues (core, async, ...); use area-vm or area-web for platform specific libraries. library-isolate P1 A high priority bug; for example, a single project is unusable or has many test failures type-enhancement A request for a change that isn't a bug
Milestone

Comments

@nex3
Copy link
Member

nex3 commented Dec 4, 2014

We'd like "pub run" to run its Dart files in an isolate rather than as a subprocess, both to avoid extra Dart VM startup time and to provide it with an accurate view of the stdio it's talking to via [stdioType]. We'd also like to run executables in checked mode when pub itself is not, and to allow users to provide various other VM flags for e.g. debugging. To support all of this, we need some sort of VM-specific means of spawning an isolate with different flags than the parent isolate.

@lrhn
Copy link
Member

lrhn commented Dec 5, 2014

Added Area-Library label.

@nex3
Copy link
Member Author

nex3 commented Dec 8, 2014

Marked this as blocking #21821.

@nex3
Copy link
Member Author

nex3 commented Feb 23, 2015

This is also important for unittest. The unittest executable isn't run in checked mode, but the isolates it loads to run tests should be.

@kevmoo
Copy link
Member

kevmoo commented Apr 23, 2015

Removed Priority-Unassigned label.
Added Priority-High label.
Changed the title to: "Add bool checkeMode optional, named arg to spawn/spawnUri isolate functions".

@kevmoo
Copy link
Member

kevmoo commented Apr 23, 2015

Blocking dart-lang/test#50


Added this to the 1.11 milestone.

@kevmoo
Copy link
Member

kevmoo commented Apr 25, 2015

Ivan: please reassign. We'd love to have this for 1.11.

Please bring me into API discussions. We'd love other flags as well.


Set owner to @iposva-google.

@iposva-google
Copy link
Contributor

Removed the owner.

@kevmoo
Copy link
Member

kevmoo commented May 3, 2015

Ivan mentioned supporting more that checked, but having checked at a minimum for 1.11 would be great for our test story.


Set owner to @sgjesse.

@sgjesse
Copy link
Contributor

sgjesse commented May 8, 2015

One thing to keep in mind is that for dart2js this will not work, so it will be specific to the actual Dart implementation. Therefore adding it as a named argument in the spawn/spawnUri might be too prominent.

Another option could be to add an optional "options" Map<String, String> argument where any arguments (e.g. '--checked' with value whatever) can be passed, and then it is implementation specific if it has any effect.


cc @lrhn.

@nex3
Copy link
Member Author

nex3 commented May 8, 2015

I wouldn't mind an options-based approach.

@kevmoo
Copy link
Member

kevmoo commented May 8, 2015

I'd argue we do want a named argument.

An options map throws away a lot of tooling help.

setting 'checked' via a named arg or a map will be equally broken on dart2js. Anything another doesn't help.


Changed the title to: "Add bool checkedMode optional, named arg to spawn/spawnUri isolate functions".

@lrhn
Copy link
Member

lrhn commented May 8, 2015

A Map<String,String> is not a good design. A better alternative would be a parameter object.
In this case, I don't have a problem with a parameter that doesn't always work, as long as it's documented that it is a request to spawn an isolate with that flag set if possible.

@iposva-google
Copy link
Contributor

And what does this Parameter object contain? A set of flags like a command line?

@lrhn
Copy link
Member

lrhn commented May 9, 2015

A Parameter Object is just an object with properties that can be passed to a function instead of passing the properties individually.

It's more useful in languages that don't have optional named parameters or default values because you can use the full power of objects instead of the, possibly limited, features available for function calls. You can have helper functions creating parameter objects pre-filled with certain combinations of arguments.

Another advantage of parameter objects is that you can extend them later without changing the signature the function, and you can pass extended parameter objects to old functions without them caring about the parameters they don't recognize.
That means that VM specific isolate flags can be added to a "VMSpawnParameters" class to a VM specific library, extending a default "SpawnParameters", and then passed to Isolate.spawnUri, without changing the signature - it's a hidden back-channel to send more parameters that might be recognized by the receiving function. Which is actually also kind-of icky :)

On the other hand, a parameter object should just be an object with a method that does what you want, instead of being a dumb object that is passed to a tightly coupled function. So the real solution would be "(new IsolateSpawner()..paused=true..packageRoot=something).spawnUri(uri)". Then you can extend IsolateSpawner if you want further functionality.

I don't think we want, or need, a parameter object here, but it beats a Map<String,String> any day.

@kwalrath
Copy link
Contributor

cc @Sfshaza.

@sgjesse
Copy link
Contributor

sgjesse commented May 29, 2015

Set owner to @lrhn.

@sethladd
Copy link
Contributor

Would love to be able to run my tests in checked mode, so greatly looking forward to this new API. Thanks for working on it!

@lrhn
Copy link
Member

lrhn commented May 31, 2015

Added Started label.

@nex3 nex3 added Type-Enhancement P1 A high priority bug; for example, a single project is unusable or has many test failures library-isolate area-core-library SDK core library issues (core, async, ...); use area-vm or area-web for platform specific libraries. labels May 31, 2015
@nex3 nex3 added this to the 1.11 milestone May 31, 2015
@kevmoo
Copy link
Member

kevmoo commented Jun 8, 2015

@lrhn is this resolved as of 5a843eb ?

@lrhn
Copy link
Member

lrhn commented Jun 8, 2015

I think so. @iposva-google landed a fix for the 5a843eb patch not really working, as f5e3f94.

@lrhn lrhn closed this as completed Jun 8, 2015
@nex3
Copy link
Member Author

nex3 commented Jun 8, 2015

lets_party-145404

@kevmoo kevmoo added type-enhancement A request for a change that isn't a bug and removed type-enhancement labels Mar 1, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-core-library SDK core library issues (core, async, ...); use area-vm or area-web for platform specific libraries. library-isolate P1 A high priority bug; for example, a single project is unusable or has many test failures type-enhancement A request for a change that isn't a bug
Projects
None yet
Development

No branches or pull requests

7 participants