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

Jump to bottom

wasi: throw on failed uvwasi_init() #31076

Merged
merged 3 commits into from Dec 26, 2019
Merged

wasi: throw on failed uvwasi_init() #31076

merged 3 commits into from Dec 26, 2019

Conversation

cjihrig
Copy link
Contributor

@cjihrig cjihrig commented Dec 24, 2019 •
edited

Prior to this commit, if uvwasi_init() failed in any way, Node would abort due to a failed CHECK_EQ(). This commit changes that behavior to a thrown exception.

Fixes: #30878

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • commit message follows commit guidelines

@nodejs-github-bot nodejs-github-bot added the c++ Issues and PRs that require attention from people who are familiar with C++. label Dec 24, 2019
@gengjiawen gengjiawen added the wasi Issues and PRs related to the WebAssembly System Interface. label Dec 24, 2019
addaleax
addaleax reviewed Dec 24, 2019
View reviewed changes
deps/uvwasi/src/fd_table.c Outdated Show resolved Hide resolved
src/node_wasi.cc Outdated Show resolved Hide resolved
src/node_wasi.cc Outdated Show resolved Hide resolved
@cjihrig
Copy link
Contributor Author

cjihrig commented Dec 24, 2019

@addaleax I think I've addressed all of your comments.

@BridgeAR BridgeAR added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Dec 24, 2019
@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/27917/

@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/27927/

@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Dec 25, 2019 •
edited by cjihrig

CI: https://ci.nodejs.org/job/node-test-pull-request/27928/ 💚

@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/27950/

@cjihrig
deps: uvwasi: cherry-pick 64e59d5
a512bf2 
Original commit message:

    This commit ensures that multiple calls to uvwasi_destroy()
    are possible. Prior to this commit, subsequent calls to
    destroy() would abort because the file table's rwlock was
    incorrectly being destroyed multiple times.

PR-URL: nodejs#31076
Reviewed-By: Jiawen Geng <technicalcute@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
@cjihrig
deps: uvwasi: cherry-pick 75b389c
6e20674 
Original commit message:

    This commit changes the memory management in
    uvwasi_fd_table_init() such that ENOMEM errors can be
    handled better in uvwasi_fd_table_free().

PR-URL: nodejs#31076
Reviewed-By: Jiawen Geng <technicalcute@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
@cjihrig
wasi: throw on failed uvwasi_init()
208453e 
Prior to this commit, if uvwasi_init() failed in any way, Node
would abort due to a failed CHECK_EQ(). This commit changes
that behavior to a thrown exception.

PR-URL: nodejs#31076
Fixes: nodejs#30878
Reviewed-By: Jiawen Geng <technicalcute@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
@cjihrig cjihrig merged commit 208453e into nodejs:master Dec 26, 2019
@cjihrig
Copy link
Contributor Author

cjihrig commented Dec 26, 2019

Landed in 3d50001...208453e. Thanks for the reviews.

@cjihrig cjihrig deleted the wasi-err branch December 26, 2019 03:28
BridgeAR pushed a commit that referenced this pull request Jan 3, 2020
@cjihrig @BridgeAR
deps: uvwasi: cherry-pick 64e59d5
a5937c7 
Original commit message:

    This commit ensures that multiple calls to uvwasi_destroy()
    are possible. Prior to this commit, subsequent calls to
    destroy() would abort because the file table's rwlock was
    incorrectly being destroyed multiple times.

PR-URL: #31076
Reviewed-By: Jiawen Geng <technicalcute@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
BridgeAR pushed a commit that referenced this pull request Jan 3, 2020
@cjihrig @BridgeAR
deps: uvwasi: cherry-pick 75b389c
2ec817e 
Original commit message:

    This commit changes the memory management in
    uvwasi_fd_table_init() such that ENOMEM errors can be
    handled better in uvwasi_fd_table_free().

PR-URL: #31076
Reviewed-By: Jiawen Geng <technicalcute@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
BridgeAR pushed a commit that referenced this pull request Jan 3, 2020
@cjihrig @BridgeAR
wasi: throw on failed uvwasi_init()
87f15c0 
Prior to this commit, if uvwasi_init() failed in any way, Node
would abort due to a failed CHECK_EQ(). This commit changes
that behavior to a thrown exception.

PR-URL: #31076
Fixes: #30878
Reviewed-By: Jiawen Geng <technicalcute@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
@BridgeAR BridgeAR mentioned this pull request Jan 7, 2020
Merged
@legendecas legendecas mentioned this pull request Jan 7, 2020
Closed
targos pushed a commit that referenced this pull request Jan 14, 2020
@cjihrig @targos
deps: uvwasi: cherry-pick 64e59d5
8b0c191 
Original commit message:

    This commit ensures that multiple calls to uvwasi_destroy()
    are possible. Prior to this commit, subsequent calls to
    destroy() would abort because the file table's rwlock was
    incorrectly being destroyed multiple times.

PR-URL: #31076
Reviewed-By: Jiawen Geng <technicalcute@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
targos pushed a commit that referenced this pull request Jan 14, 2020
@cjihrig @targos
deps: uvwasi: cherry-pick 75b389c
c5deed1 
Original commit message:

    This commit changes the memory management in
    uvwasi_fd_table_init() such that ENOMEM errors can be
    handled better in uvwasi_fd_table_free().

PR-URL: #31076
Reviewed-By: Jiawen Geng <technicalcute@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
targos pushed a commit that referenced this pull request Jan 14, 2020
@cjihrig @targos
wasi: throw on failed uvwasi_init()
a48b946 
Prior to this commit, if uvwasi_init() failed in any way, Node
would abort due to a failed CHECK_EQ(). This commit changes
that behavior to a thrown exception.

PR-URL: #31076
Fixes: #30878
Reviewed-By: Jiawen Geng <technicalcute@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
targos pushed a commit that referenced this pull request Jan 14, 2020
@cjihrig @targos
deps: uvwasi: cherry-pick 64e59d5
a9d9fdb 
Original commit message:

    This commit ensures that multiple calls to uvwasi_destroy()
    are possible. Prior to this commit, subsequent calls to
    destroy() would abort because the file table's rwlock was
    incorrectly being destroyed multiple times.

PR-URL: #31076
Reviewed-By: Jiawen Geng <technicalcute@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
targos pushed a commit that referenced this pull request Jan 14, 2020
@cjihrig @targos
deps: uvwasi: cherry-pick 75b389c
6784296 
Original commit message:

    This commit changes the memory management in
    uvwasi_fd_table_init() such that ENOMEM errors can be
    handled better in uvwasi_fd_table_free().

PR-URL: #31076
Reviewed-By: Jiawen Geng <technicalcute@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
targos pushed a commit that referenced this pull request Jan 14, 2020
@cjihrig @targos
wasi: throw on failed uvwasi_init()
e815655 
Prior to this commit, if uvwasi_init() failed in any way, Node
would abort due to a failed CHECK_EQ(). This commit changes
that behavior to a thrown exception.

PR-URL: #31076
Fixes: #30878
Reviewed-By: Jiawen Geng <technicalcute@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
@targos targos mentioned this pull request Jan 15, 2020
Closed
BethGriggs pushed a commit that referenced this pull request Feb 6, 2020
@cjihrig @BethGriggs
deps: uvwasi: cherry-pick 64e59d5
8f4339b 
Original commit message:

    This commit ensures that multiple calls to uvwasi_destroy()
    are possible. Prior to this commit, subsequent calls to
    destroy() would abort because the file table's rwlock was
    incorrectly being destroyed multiple times.

PR-URL: #31076
Reviewed-By: Jiawen Geng <technicalcute@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
BethGriggs pushed a commit that referenced this pull request Feb 6, 2020
@cjihrig @BethGriggs
deps: uvwasi: cherry-pick 75b389c
6de77d3 
Original commit message:

    This commit changes the memory management in
    uvwasi_fd_table_init() such that ENOMEM errors can be
    handled better in uvwasi_fd_table_free().

PR-URL: #31076
Reviewed-By: Jiawen Geng <technicalcute@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
BethGriggs pushed a commit that referenced this pull request Feb 6, 2020
@cjihrig @BethGriggs
wasi: throw on failed uvwasi_init()
0d212fc 
Prior to this commit, if uvwasi_init() failed in any way, Node
would abort due to a failed CHECK_EQ(). This commit changes
that behavior to a thrown exception.

PR-URL: #31076
Fixes: #30878
Reviewed-By: Jiawen Geng <technicalcute@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
@MylesBorins MylesBorins mentioned this pull request Feb 8, 2020
Merged
mhdawson added a commit to mhdawson/io.js that referenced this pull request Sep 25, 2023
@mhdawson
wasi: address coverity warning
6818c22 
- add check for case when trying to provide
  a better Exception fails
- the code was modified to avoid a CHECK_EQ in all
  cases in nodejs#31076,
  however, I believe that if we fail to create the exeption
  to throw instead of simply returning using a CHECK makes
  more sense. I think it should also address the coverity
  warning about not initializing in the constructor.

Signed-off-by: Michael Dawson <midawson@redhat.com>
@mhdawson mhdawson mentioned this pull request Sep 25, 2023
Closed
mhdawson added a commit that referenced this pull request Oct 12, 2023
@mhdawson
wasi: address coverity warning
c1a3a98 
- add check for case when trying to provide
  a better Exception fails
- the code was modified to avoid a CHECK_EQ in all
  cases in #31076,
  however, I believe that if we fail to create the exeption
  to throw instead of simply returning using a CHECK makes
  more sense. I think it should also address the coverity
  warning about not initializing in the constructor.

Signed-off-by: Michael Dawson <midawson@redhat.com>

PR-URL: #49866
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
alexfernandez pushed a commit to alexfernandez/node that referenced this pull request Nov 1, 2023
@mhdawson @alexfernandez
wasi: address coverity warning
6e88292 
- add check for case when trying to provide
  a better Exception fails
- the code was modified to avoid a CHECK_EQ in all
  cases in nodejs#31076,
  however, I believe that if we fail to create the exeption
  to throw instead of simply returning using a CHECK makes
  more sense. I think it should also address the coverity
  warning about not initializing in the constructor.

Signed-off-by: Michael Dawson <midawson@redhat.com>

PR-URL: nodejs#49866
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
targos pushed a commit that referenced this pull request Nov 11, 2023
@mhdawson @targos
wasi: address coverity warning
65e18aa 
- add check for case when trying to provide
  a better Exception fails
- the code was modified to avoid a CHECK_EQ in all
  cases in #31076,
  however, I believe that if we fail to create the exeption
  to throw instead of simply returning using a CHECK makes
  more sense. I think it should also address the coverity
  warning about not initializing in the constructor.

Signed-off-by: Michael Dawson <midawson@redhat.com>

PR-URL: #49866
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gengjiawen gengjiawen gengjiawen approved these changes

@jasnell jasnell jasnell approved these changes

@Trott Trott Trott approved these changes

@addaleax addaleax addaleax approved these changes

@legendecas legendecas legendecas approved these changes

Assignees
No one assigned
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. c++ Issues and PRs that require attention from people who are familiar with C++. wasi Issues and PRs related to the WebAssembly System Interface.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

better error message for wasi
8 participants
@cjihrig @nodejs-github-bot @jasnell @Trott @addaleax @gengjiawen @legendecas @BridgeAR