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

fest(std/node): implement os.getPriority() and os.setPriority() #4202

Closed
wants to merge 4 commits into from

Conversation

ecyrbe
Copy link
Contributor

@ecyrbe ecyrbe commented Mar 1, 2020

this PR implement node os priority feature. see #3802 and #3403 .

It looks like it adds errno dependency, but is was already used by our current dependencies and listed in cargo.lock.

it uses our current dependencies to implement these functionalities :

  • unsafe libc for unix
  • unsafe winapi for windows

a new internal crate (file priority.rs) was created to hide implementation the details from ops part.

@ecyrbe ecyrbe force-pushed the feature/std-node-os-priority branch 2 times, most recently from 0d44fa6 to 3125532 Compare March 1, 2020 04:19
@cknight
Copy link
Contributor

cknight commented Mar 1, 2020

Looks like the test for API not yet implemented still contains calls to os.getPriority() and os.setPriority(). Hopefully those tests now fail! See lines 212 and 226 in std/node/os_test.ts.

@ecyrbe
Copy link
Contributor Author

ecyrbe commented Mar 1, 2020

Looks like the test for API not yet implemented still contains calls to os.getPriority() and os.setPriority(). Hopefully those tests now fail! See lines 212 and 226 in std/node/os_test.ts.

Thanks for the catch. the test for the api was implemented, but these one where not removed.

@ecyrbe ecyrbe force-pushed the feature/std-node-os-priority branch 2 times, most recently from 7065898 to 947653f Compare March 1, 2020 10:19
@ecyrbe ecyrbe changed the title fest(std/node): implement os.getPriority() ans os.setPriority() fest(std/node): implement os.getPriority() and os.setPriority() Mar 1, 2020
@ecyrbe ecyrbe force-pushed the feature/std-node-os-priority branch from 3aa15b9 to f180e6f Compare March 1, 2020 11:37
@ecyrbe ecyrbe force-pushed the feature/std-node-os-priority branch 2 times, most recently from 8ca3bce to f8994cb Compare March 1, 2020 15:07
@ecyrbe
Copy link
Contributor Author

ecyrbe commented Mar 1, 2020

@cknight @ry this is ready for review if you want

cli/js/lib.deno.ns.d.ts Outdated Show resolved Hide resolved
cli/js/lib.deno.ns.d.ts Outdated Show resolved Hide resolved
cli/js/os_test.ts Outdated Show resolved Hide resolved
cli/js/os_test.ts Outdated Show resolved Hide resolved
cli/priority.rs Outdated Show resolved Hide resolved
@cknight
Copy link
Contributor

cknight commented Mar 1, 2020

Thanks @ecyrbe, I've added a few suggestions, but will leave the rust review to @ry.

One further suggestion, what about making the different priority levels exposed as an enum, similiar to https://nodejs.org/api/os.html#os_priority_constants? This would make it more user friendly.

Then the API could be something like:

setPriority(priority: number | OsPriority, pid?:number)

@ecyrbe
Copy link
Contributor Author

ecyrbe commented Mar 1, 2020

Thanks @ecyrbe, I've added a few suggestions, but will leave the rust review to @ry.

One further suggestion, what about making the different priority levels exposed as an enum, similiar to https://nodejs.org/api/os.html#os_priority_constants? This would make it more user friendly.

Then the API could be something like:

setPriority(priority: number | OsPriority, pid?:number)

i'll definitly add this to Deno api layer to ease deno developpement.

cli/js/lib.deno.ns.d.ts Outdated Show resolved Hide resolved
cli/js/lib.deno.ns.d.ts Outdated Show resolved Hide resolved
cli/js/lib.deno.ns.d.ts Outdated Show resolved Hide resolved
cli/js/os.ts Outdated Show resolved Hide resolved
cli/js/os.ts Outdated Show resolved Hide resolved
cli/js/os.ts Outdated Show resolved Hide resolved
cli/priority.rs Outdated Show resolved Hide resolved
@cknight
Copy link
Contributor

cknight commented Mar 1, 2020

Thanks, LGTM once the CI passes. Rust code still needs reviewed by someone with Rust experience.

@ecyrbe ecyrbe force-pushed the feature/std-node-os-priority branch from 8107809 to 75ffc07 Compare March 2, 2020 00:08
@ecyrbe ecyrbe force-pushed the feature/std-node-os-priority branch from 75ffc07 to cff74d8 Compare March 2, 2020 00:39
Copy link
Member

@ry ry left a comment

Choose a reason for hiding this comment

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

Looks good! Just a few minor comments...

cli/js/lib.deno.ns.d.ts Show resolved Hide resolved
cli/priority.rs Show resolved Hide resolved
cli/priority.rs Show resolved Hide resolved
Copy link
Member

@ry ry left a comment

Choose a reason for hiding this comment

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

I'm not sure what permissions this needs, but surely both set and getPriority should not be allowed in the default mode.

@ecyrbe
Copy link
Contributor Author

ecyrbe commented Mar 3, 2020

Should i set Env permissions ?

@ecyrbe
Copy link
Contributor Author

ecyrbe commented Mar 4, 2020

@ry Or should we create a new permission level? like :

  • allow_api
  • api_whitelist

@ecyrbe
Copy link
Contributor Author

ecyrbe commented Mar 8, 2020

@ry just a reminder, could you give me a hint at what route i should take for permissions ? see above comment.

@ry
Copy link
Member

ry commented Mar 9, 2020

We've discussed this. For now we'd like to require --allow-all in order to issue these calls. This is niche functionality and it certainly requires permissions, we're not keen on adding a new class of permissions just for this at this time. In the future, if people want it, we can introduce some more precision here - but --allow-all is a safe way to move forward.

@ecyrbe
Copy link
Contributor Author

ecyrbe commented Mar 9, 2020

thanks. i agree, i'll add it then.

@ry
Copy link
Member

ry commented May 4, 2020

@ecyrbe I'm sorry we couldn't get this in. I'm going to close this because it's stale. In the future when we try to implement this again we will refer back to this patch. But at the moment the permissions situation is unresolved and the patch is now quite out of date. I'm going to close this because it's stale. I appreciate the work - it sucks when we don't land nice patches like this.

@ry ry closed this May 4, 2020
crowlKats added a commit that referenced this pull request Jul 31, 2023
Takes #4202 over
Closes #17850 

---------

Co-authored-by: ecyrbe <ecyrbe@gmail.com>
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.

3 participants