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

win: fix unsavory rwlock fallback implementation #516

Merged
merged 1 commit into from Sep 11, 2015

Conversation

piscisaureus
Copy link
Contributor

Before this patch an uv_mutex_t (backed by a critical section) could be
released by a tread different from the thread that acquired it, which is
not allowed. This is fixed by using a semaphore instead.

BUG: #515
REF: nodejs/node#2723

R=@bnoordhuis
R=@saghul

piscisaureus added a commit to piscisaureus/libuv that referenced this pull request Sep 8, 2015
Before this patch an uv_mutex_t (backed by a critical section) could be
released by a tread different from the thread that acquired it, which is
not allowed. This is fixed by using a semaphore instead.

Fixes: libuv#515
PR-URL: libuv#516
err = uv_mutex_init(&rwlock->fallback_.read_mutex_);
if (err)
return err;
// Initialize the semaphore that acts as the write lock.
Copy link
Member

Choose a reason for hiding this comment

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

/* */

@saghul
Copy link
Member

saghul commented Sep 8, 2015

LGTM, sans the c++ style comments, and a couple of small questions.


if (++rwlock->fallback_.num_readers_ == 1)
uv_mutex_lock(&rwlock->fallback_.write_mutex_);
// Acquire the lock that protects the reader count;
Copy link
Member

Choose a reason for hiding this comment

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

And ditto.

piscisaureus added a commit to piscisaureus/libuv that referenced this pull request Sep 10, 2015
Before this patch an uv_mutex_t (backed by a critical section) could be
released by a tread different from the thread that acquired it, which is
not allowed. This is fixed by using a semaphore instead.

Fixes: libuv#515
PR-URL: libuv#516
@piscisaureus
Copy link
Contributor Author

Fixed the comment style and the typos.

@saghul, let me know what you want to do with the WaitForSingleObject() failure modes. I think it's fine as it is, all those "failure" scenarios are purely hypothetical anyway. My thinking was that I would return a mapped error from every function that actually returns a value, and abort (by means of uv_fatal_error) in functions that return void.

cc @bnoordhuis

@saghul
Copy link
Member

saghul commented Sep 10, 2015

LGTM. Let's leave it as is then.

piscisaureus added a commit to piscisaureus/libuv that referenced this pull request Sep 11, 2015
Before this patch an uv_mutex_t (backed by a critical section) could be
released by a tread different from the thread that acquired it, which is
not allowed. This is fixed by using a semaphore instead.

Note that the affected code paths were used on Windows XP and Windows
Server 2003 only.

Fixes: libuv#515
PR-URL: libuv#516
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Saúl Ibarra Corretgé <saghul@gmail.com>
Before this patch an uv_mutex_t (backed by a critical section) could be
released by a tread different from the thread that acquired it, which is
not allowed. This is fixed by using a semaphore instead.

Note that the affected code paths were used on Windows XP and Windows
Server 2003 only.

Fixes: libuv#515
PR-URL: libuv#516
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Saúl Ibarra Corretgé <saghul@gmail.com>
@piscisaureus piscisaureus merged commit 3eb6764 into libuv:v1.x Sep 11, 2015
@piscisaureus piscisaureus deleted the win-rwlock branch September 11, 2015 04:06
evanlucas pushed a commit to evanlucas/libuv that referenced this pull request Dec 5, 2015
Before this patch an uv_mutex_t (backed by a critical section) could be
released by a tread different from the thread that acquired it, which is
not allowed. This is fixed by using a semaphore instead.

Note that the affected code paths were used on Windows XP and Windows
Server 2003 only.

Fixes: libuv#515
PR-URL: libuv#516
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Saúl Ibarra Corretgé <saghul@gmail.com>
jBarz pushed a commit to ibmruntimes/node that referenced this pull request Nov 4, 2016
Original commit message:

    win: fix unsavory rwlock fallback implementation

    Before this patch an uv_mutex_t (backed by a critical section) could be
    released by a tread different from the thread that acquired it, which is
    not allowed. This is fixed by using a semaphore instead.

    Note that the affected code paths were used on Windows XP and Windows
    Server 2003 only.

    Fixes: libuv/libuv#515
    PR-URL: libuv/libuv#516
    Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
    Reviewed-By: Saúl Ibarra Corretgé <saghul@gmail.com>

PR-URL: https://github.com/nodejs/node-private/pull/54
Reviewed-By: Saúl Ibarra Corretgé <saghul@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.

None yet

3 participants