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

Proposal: Remove Deno.umask (with no argument call) #13632

Closed
kt3k opened this issue Feb 9, 2022 · 6 comments
Closed

Proposal: Remove Deno.umask (with no argument call) #13632

kt3k opened this issue Feb 9, 2022 · 6 comments
Labels
proposal A proposal for a new feature or change stale

Comments

@kt3k
Copy link
Member

kt3k commented Feb 9, 2022

Deno.umask() (no argument) has a race codition bug. Deno.umask() (no argument) uses 2 syscalls and between these 2, there's small amount of time when the umask value is incorrect, and mkdir can create a directory with wrong permission on that timing.

If we try to address the above issue properly, we probably need mutex lock for fs op calls. But that introduces much complexity, and also causes negative performance impact.

Also umask is a global state, and not something we should encourage using in general situations i.e. it can be considered as an antipattern. So it doesn't make much sense to introduce new complexity to support this feature. So I propose we should remove Deno.umask.

I propose we should remove Deno.umask with no argument

@kitsonk kitsonk added the proposal A proposal for a new feature or change label Feb 9, 2022
@kt3k kt3k changed the title Proposal: Remove (unstable) Deno.umask Proposal: Remove Deno.umask (with no argument call) Feb 15, 2022
@kt3k
Copy link
Member Author

kt3k commented Feb 15, 2022

Updated the proposal. Now it proposes only the removal of no argument version of Deno.umask().

@stale
Copy link

stale bot commented Apr 18, 2022

This issue has been automatically marked as stale because it has not had recent activity. It will be closed in 7 days if no further activity occurs. Thank you for your contributions.

@stale stale bot added the stale label Apr 18, 2022
@kt3k kt3k removed the stale label Apr 19, 2022
@stale
Copy link

stale bot commented Jun 18, 2022

This issue has been automatically marked as stale because it has not had recent activity. It will be closed in 7 days if no further activity occurs. Thank you for your contributions.

@stale stale bot added the stale label Jun 18, 2022
@kt3k
Copy link
Member Author

kt3k commented Jun 19, 2022

There are still ideas to improve Deno.umask (I think Bert has more details about it), but that isn't like the way described in this proposal. So I'm closing this for now.

BTW the problem of umask first affected deno compat mode because of it's racy nature, but now we work around it by providing mock implementation to process.umask in std/node https://github.com/denoland/deno_std/blob/d697527754d17cb911671d82104191515b14f1cd/node/process.ts#L501 . So the priority of this issue is now very low

@kt3k kt3k closed this as completed Jun 19, 2022
@cjihrig
Copy link
Contributor

cjihrig commented Jun 19, 2022

FWIW, Node's process.umask() with no arguments is officially deprecated due to a race condition.

@lucacasonato
Copy link
Member

I am still in favor of landing this FWIW.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
proposal A proposal for a new feature or change stale
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants