Skip to content

RM_ThreadSafeContextTryLock a non-blocking method for acquiring GIL#7738

Merged
oranagra merged 2 commits intoredis:unstablefrom
swilly22:ThreadSafeContextTryLock
Sep 9, 2020
Merged

RM_ThreadSafeContextTryLock a non-blocking method for acquiring GIL#7738
oranagra merged 2 commits intoredis:unstablefrom
swilly22:ThreadSafeContextTryLock

Conversation

@swilly22
Copy link
Contributor

@swilly22 swilly22 commented Sep 1, 2020

No description provided.

Copy link
Member

@oranagra oranagra left a comment

Choose a reason for hiding this comment

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

i wanna reduce the number of units and modules we have (less boilerplate code)

i think it would be better to rename the test unit and module to something more generic so that it'll be natural to add more tests to it in the future.
i see we have one test named blockedkeys, so maybe rename this one to blockedclient?

another alternative is to rename the existing test to just blocked so that we can merge them both into the same module and unit?

@swilly22 swilly22 force-pushed the ThreadSafeContextTryLock branch 5 times, most recently from f93a45e to dce1864 Compare September 2, 2020 14:16
oranagra
oranagra previously approved these changes Sep 2, 2020
@oranagra oranagra added the state:major-decision Requires core team consensus label Sep 2, 2020
@oranagra
Copy link
Member

oranagra commented Sep 2, 2020

@redis/core-team adding a trivial API, please approve.

@oranagra oranagra added the state:to-be-merged The PR should be merged soon, even if not yet ready, this is used so that it won't be forgotten label Sep 2, 2020
itamarhaber
itamarhaber previously approved these changes Sep 2, 2020
@oranagra
Copy link
Member

oranagra commented Sep 3, 2020

Two more requests:

  1. Let's add an optional timeout argument (using pthread_mutex_timedlock) so that we don't have to add another API in the future.
  2. Let's store the return value into errno so that users can distinguish between the different types of errors (we already have other APIs doing that)

@yossigo
Copy link
Collaborator

yossigo commented Sep 3, 2020

@oranagra Why manipulate errno and not have a proper API to return errors?

@oranagra
Copy link
Member

oranagra commented Sep 4, 2020

IMHO it feels more natural and matches the rest of the API. We don't have any module API that returns errno (yet).
Come to think of it most of the POSIX api returns 0 or non-0 and the error comes from errno. So why is the mutex API different?

@yossigo
Copy link
Collaborator

yossigo commented Sep 6, 2020

After discussing this with @oranagra, our conclusions are:

  1. Using errno is somewhat of a code smell but it's already the practice in other Module API functions so we'll stick with that for the sake of consistency.
  2. We need an optional timeout argument as described above.
    @swilly22 can you incorporate these changes into the PR?

@swilly22
Copy link
Contributor Author

swilly22 commented Sep 6, 2020

@yossigo sure thing.

@swilly22 swilly22 dismissed stale reviews from itamarhaber and oranagra via 34a70fa September 6, 2020 14:20
oranagra
oranagra previously approved these changes Sep 6, 2020
@swilly22
Copy link
Contributor Author

swilly22 commented Sep 6, 2020

Turns out pthread_mutex_timedlock isn’t supported on the following platforms:

Mac OS X 10.13, FreeBSD 5.2.1, NetBSD 7.1, OpenBSD 3.8, Minix 3.1.8, AIX 5.1, HP-UX 11, IRIX 6.5, Solaris 9, Cygwin 1.7.x, mingw, MSVC 14, Android 4.4.

This is a suggested workaround:

#if defined(_POSIX_TIMEOUTS)  && _POSIX_TIMEOUTS >= 200112L
   r = pthread_mutex_timedlock(&mutList, &to);
#else
   r = pthread_mutex_trylock(&mutList); // must check this
#endif

In this case I suggest we revert.

@oranagra
Copy link
Member

oranagra commented Sep 6, 2020

yes, let's revert..
the only other option is to add a separate module API for timedlock, which may or may not exist, and have modules check for it's existence at runtime before using.

@yossigo
Copy link
Collaborator

yossigo commented Sep 8, 2020

@oranagra I am not in favor of a Module API that is OS specific and may or may not be available regardless of Redis version. We can just not support timeouts.

oranagra
oranagra previously approved these changes Sep 8, 2020
@oranagra
Copy link
Member

oranagra commented Sep 8, 2020

@redis/core-team please approve.

yossigo
yossigo previously approved these changes Sep 8, 2020
Copy link
Collaborator

@yossigo yossigo left a comment

Choose a reason for hiding this comment

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

Added a couple of nitpick comments, not mandatory.

@oranagra oranagra dismissed stale reviews from yossigo and themself via 14a12cc September 9, 2020 11:42
@oranagra oranagra changed the title ThreadContextTryLock a none blocking method for acquiring GIL RM_ThreadSafeContextTryLock a non-blocking method for acquiring GIL Sep 9, 2020
@oranagra oranagra merged commit 042189f into redis:unstable Sep 9, 2020
@swilly22 swilly22 deleted the ThreadSafeContextTryLock branch September 9, 2020 13:03
oranagra added a commit that referenced this pull request Sep 10, 2020
…7738)

Co-authored-by: Yossi Gottlieb <[email protected]>
Co-authored-by: Oran Agra <[email protected]>
(cherry picked from commit 042189f)
JackieXie168 pushed a commit to JackieXie168/redis that referenced this pull request Nov 4, 2020
jschmieg pushed a commit to memKeyDB/memKeyDB that referenced this pull request Nov 6, 2020
…edis#7738)

Co-authored-by: Yossi Gottlieb <[email protected]>
Co-authored-by: Oran Agra <[email protected]>
(cherry picked from commit 042189f)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

state:major-decision Requires core team consensus state:to-be-merged The PR should be merged soon, even if not yet ready, this is used so that it won't be forgotten

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants