You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@brpc.apache.org by GitBox <gi...@apache.org> on 2023/01/15 14:04:43 UTC
[GitHub] [brpc] jenrryyou opened a new pull request, #2086: Fix butex wait_pthread handle EINTR
jenrryyou opened a new pull request, #2086:
URL: https://github.com/apache/brpc/pull/2086
wait_pthread处理被信号中断的情况。
没有这个处理之前，假设butex_wait_from_pthread的timeout设置为100s，但是这个线程每隔10s会被信号中断（如正在进行profile，会定时中断采集线程堆栈），futex_wait_private会返回EINTR，导致butex_wait_from_pthread永远不会超时返回。
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For additional commands, e-mail: dev-help@brpc.apache.org
[GitHub] [brpc] wwbmmm commented on pull request #2086: Fix butex wait_pthread handle EINTR
Posted by "wwbmmm (via GitHub)" <gi...@apache.org>.
wwbmmm commented on PR #2086:
URL: https://github.com/apache/brpc/pull/2086#issuecomment-1401638555
LGTM
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For additional commands, e-mail: dev-help@brpc.apache.org
[GitHub] [brpc] jamesge commented on pull request #2086: Fix butex wait_pthread handle EINTR
Posted by "jamesge (via GitHub)" <gi...@apache.org>.
jamesge commented on PR #2086:
URL: https://github.com/apache/brpc/pull/2086#issuecomment-1403128526
能为这种情况增加一个单测么？（bthread的修改可能影响面很大）
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For additional commands, e-mail: dev-help@brpc.apache.org
[GitHub] [brpc] jenrryyou commented on a diff in pull request #2086: Fix butex wait_pthread handle EINTR
Posted by "jenrryyou (via GitHub)" <gi...@apache.org>.
jenrryyou commented on code in PR #2086:
URL: https://github.com/apache/brpc/pull/2086#discussion_r1084834153
##########
src/bthread/butex.cpp:
##########
@@ -137,27 +137,45 @@ static void wakeup_pthread(ButexPthreadWaiter* pw) {
bool erase_from_butex(ButexWaiter*, bool, WaiterState);
-int wait_pthread(ButexPthreadWaiter& pw, timespec* ptimeout) {
+int wait_pthread(ButexPthreadWaiter& pw, const timespec* abstime) {
+ timespec * ptimeout = NULL;
+ timespec timeout;
+ int64_t timeout_us = 0;
+ int rc;
+
while (true) {
- const int rc = futex_wait_private(&pw.sig, PTHREAD_NOT_SIGNALLED, ptimeout);
- if (PTHREAD_NOT_SIGNALLED != pw.sig.load(butil::memory_order_acquire)) {
- // If `sig' is changed, wakeup_pthread() must be called and `pw'
- // is already removed from the butex.
- // Acquire fence makes this thread sees changes before wakeup.
- return rc;
+ if (abstime != NULL) {
+ timeout_us = butil::timespec_to_microseconds(*abstime) - butil::gettimeofday_us();
+ timeout = butil::microseconds_to_timespec(timeout_us);
+ ptimeout = &timeout;
}
- if (rc != 0 && errno == ETIMEDOUT) {
- // Note that we don't handle the EINTR from futex_wait here since
- // pthreads waiting on a butex should behave similarly as bthreads
- // which are not able to be woken-up by signals.
- // EINTR on butex is only producible by TaskGroup::interrupt().
-
- // `pw' is still in the queue, remove it.
+ if (timeout_us > MIN_SLEEP_US || abstime == NULL) {
+ rc = futex_wait_private(&pw.sig, PTHREAD_NOT_SIGNALLED, ptimeout);
+ if (PTHREAD_NOT_SIGNALLED != pw.sig.load(butil::memory_order_acquire)) {
+ // If `sig' is changed, wakeup_pthread() must be called and `pw'
+ // is already removed from the butex.
+ // Acquire fence makes this thread sees changes before wakeup.
+ return rc;
+ }
+ }
+ else {
+ errno = ETIMEDOUT;
+ rc = -1;
+ }
+ // Handle EINTR or ETIMEDOUT when abstime is valid
+ if (rc != 0 && abstime != NULL) {
+ // Handle the EINTR from futex_wait
+ if (errno != ETIMEDOUT) {
Review Comment:
已修改
##########
src/bthread/butex.cpp:
##########
@@ -137,27 +137,45 @@ static void wakeup_pthread(ButexPthreadWaiter* pw) {
bool erase_from_butex(ButexWaiter*, bool, WaiterState);
-int wait_pthread(ButexPthreadWaiter& pw, timespec* ptimeout) {
+int wait_pthread(ButexPthreadWaiter& pw, const timespec* abstime) {
+ timespec * ptimeout = NULL;
Review Comment:
已修改
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For additional commands, e-mail: dev-help@brpc.apache.org
[GitHub] [brpc] wwbmmm commented on a diff in pull request #2086: Fix butex wait_pthread handle EINTR
Posted by GitBox <gi...@apache.org>.
wwbmmm commented on code in PR #2086:
URL: https://github.com/apache/brpc/pull/2086#discussion_r1070816953
##########
src/bthread/butex.cpp:
##########
@@ -137,7 +137,21 @@ static void wakeup_pthread(ButexPthreadWaiter* pw) {
bool erase_from_butex(ButexWaiter*, bool, WaiterState);
-int wait_pthread(ButexPthreadWaiter& pw, timespec* ptimeout) {
+int wait_pthread(ButexPthreadWaiter& pw, const timespec* abstime) {
+ timespec * ptimeout = NULL;
+ timespec timeout;
+ int64_t timeout_us;
+
+ if (abstime != NULL) {
+ timeout_us = butil::timespec_to_microseconds(*abstime) - butil::gettimeofday_us();
+ if (timeout_us < MIN_SLEEP_US) {
+ errno = ETIMEDOUT;
+ return -1;
+ }
+ timeout = butil::microseconds_to_timespec(timeout_us);
+ ptimeout = &timeout;
+ }
+
while (true) {
const int rc = futex_wait_private(&pw.sig, PTHREAD_NOT_SIGNALLED, ptimeout);
Review Comment:
可以把计算ptimeout的逻辑移到while里面，futex_wait_private的前面，这样可以减少重复逻辑
##########
src/bthread/butex.cpp:
##########
@@ -137,7 +137,21 @@ static void wakeup_pthread(ButexPthreadWaiter* pw) {
bool erase_from_butex(ButexWaiter*, bool, WaiterState);
-int wait_pthread(ButexPthreadWaiter& pw, timespec* ptimeout) {
+int wait_pthread(ButexPthreadWaiter& pw, const timespec* abstime) {
+ timespec * ptimeout = NULL;
+ timespec timeout;
+ int64_t timeout_us;
+
+ if (abstime != NULL) {
+ timeout_us = butil::timespec_to_microseconds(*abstime) - butil::gettimeofday_us();
+ if (timeout_us < MIN_SLEEP_US) {
+ errno = ETIMEDOUT;
+ return -1;
Review Comment:
这里直接return，没走erase_from_butex的逻辑，是不是不对。
##########
src/bthread/butex.cpp:
##########
@@ -146,13 +160,19 @@ int wait_pthread(ButexPthreadWaiter& pw, timespec* ptimeout) {
// Acquire fence makes this thread sees changes before wakeup.
return rc;
}
- if (rc != 0 && errno == ETIMEDOUT) {
- // Note that we don't handle the EINTR from futex_wait here since
- // pthreads waiting on a butex should behave similarly as bthreads
- // which are not able to be woken-up by signals.
- // EINTR on butex is only producible by TaskGroup::interrupt().
+ if (rc != 0 && ptimeout != NULL) {
+ // Handle the EINTR from futex_wait
+ if (errno != ETIMEDOUT)
+ {
Review Comment:
{ 不用换行
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For additional commands, e-mail: dev-help@brpc.apache.org
[GitHub] [brpc] jenrryyou commented on a diff in pull request #2086: Fix butex wait_pthread handle EINTR
Posted by GitBox <gi...@apache.org>.
jenrryyou commented on code in PR #2086:
URL: https://github.com/apache/brpc/pull/2086#discussion_r1073469472
##########
src/bthread/butex.cpp:
##########
@@ -137,7 +137,21 @@ static void wakeup_pthread(ButexPthreadWaiter* pw) {
bool erase_from_butex(ButexWaiter*, bool, WaiterState);
-int wait_pthread(ButexPthreadWaiter& pw, timespec* ptimeout) {
+int wait_pthread(ButexPthreadWaiter& pw, const timespec* abstime) {
+ timespec * ptimeout = NULL;
+ timespec timeout;
+ int64_t timeout_us;
+
+ if (abstime != NULL) {
+ timeout_us = butil::timespec_to_microseconds(*abstime) - butil::gettimeofday_us();
+ if (timeout_us < MIN_SLEEP_US) {
+ errno = ETIMEDOUT;
+ return -1;
+ }
+ timeout = butil::microseconds_to_timespec(timeout_us);
+ ptimeout = &timeout;
+ }
+
while (true) {
const int rc = futex_wait_private(&pw.sig, PTHREAD_NOT_SIGNALLED, ptimeout);
Review Comment:
已修改
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For additional commands, e-mail: dev-help@brpc.apache.org
[GitHub] [brpc] jenrryyou commented on a diff in pull request #2086: Fix butex wait_pthread handle EINTR
Posted by GitBox <gi...@apache.org>.
jenrryyou commented on code in PR #2086:
URL: https://github.com/apache/brpc/pull/2086#discussion_r1073469170
##########
src/bthread/butex.cpp:
##########
@@ -137,7 +137,21 @@ static void wakeup_pthread(ButexPthreadWaiter* pw) {
bool erase_from_butex(ButexWaiter*, bool, WaiterState);
-int wait_pthread(ButexPthreadWaiter& pw, timespec* ptimeout) {
+int wait_pthread(ButexPthreadWaiter& pw, const timespec* abstime) {
+ timespec * ptimeout = NULL;
+ timespec timeout;
+ int64_t timeout_us;
+
+ if (abstime != NULL) {
+ timeout_us = butil::timespec_to_microseconds(*abstime) - butil::gettimeofday_us();
+ if (timeout_us < MIN_SLEEP_US) {
+ errno = ETIMEDOUT;
+ return -1;
Review Comment:
已经修改，感谢指出
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For additional commands, e-mail: dev-help@brpc.apache.org
[GitHub] [brpc] wwbmmm commented on a diff in pull request #2086: Fix butex wait_pthread handle EINTR
Posted by "wwbmmm (via GitHub)" <gi...@apache.org>.
wwbmmm commented on code in PR #2086:
URL: https://github.com/apache/brpc/pull/2086#discussion_r1083705058
##########
src/bthread/butex.cpp:
##########
@@ -137,27 +137,45 @@ static void wakeup_pthread(ButexPthreadWaiter* pw) {
bool erase_from_butex(ButexWaiter*, bool, WaiterState);
-int wait_pthread(ButexPthreadWaiter& pw, timespec* ptimeout) {
+int wait_pthread(ButexPthreadWaiter& pw, const timespec* abstime) {
+ timespec * ptimeout = NULL;
+ timespec timeout;
+ int64_t timeout_us = 0;
+ int rc;
+
while (true) {
- const int rc = futex_wait_private(&pw.sig, PTHREAD_NOT_SIGNALLED, ptimeout);
- if (PTHREAD_NOT_SIGNALLED != pw.sig.load(butil::memory_order_acquire)) {
- // If `sig' is changed, wakeup_pthread() must be called and `pw'
- // is already removed from the butex.
- // Acquire fence makes this thread sees changes before wakeup.
- return rc;
+ if (abstime != NULL) {
+ timeout_us = butil::timespec_to_microseconds(*abstime) - butil::gettimeofday_us();
+ timeout = butil::microseconds_to_timespec(timeout_us);
+ ptimeout = &timeout;
}
- if (rc != 0 && errno == ETIMEDOUT) {
- // Note that we don't handle the EINTR from futex_wait here since
- // pthreads waiting on a butex should behave similarly as bthreads
- // which are not able to be woken-up by signals.
- // EINTR on butex is only producible by TaskGroup::interrupt().
-
- // `pw' is still in the queue, remove it.
+ if (timeout_us > MIN_SLEEP_US || abstime == NULL) {
+ rc = futex_wait_private(&pw.sig, PTHREAD_NOT_SIGNALLED, ptimeout);
+ if (PTHREAD_NOT_SIGNALLED != pw.sig.load(butil::memory_order_acquire)) {
+ // If `sig' is changed, wakeup_pthread() must be called and `pw'
+ // is already removed from the butex.
+ // Acquire fence makes this thread sees changes before wakeup.
+ return rc;
+ }
+ }
+ else {
+ errno = ETIMEDOUT;
+ rc = -1;
+ }
+ // Handle EINTR or ETIMEDOUT when abstime is valid
+ if (rc != 0 && abstime != NULL) {
+ // Handle the EINTR from futex_wait
+ if (errno != ETIMEDOUT) {
Review Comment:
可以把上一个判断恢复成rc != 0 && errno == ETIMEDOUT。这里就不用再判断了。
##########
src/bthread/butex.cpp:
##########
@@ -137,27 +137,45 @@ static void wakeup_pthread(ButexPthreadWaiter* pw) {
bool erase_from_butex(ButexWaiter*, bool, WaiterState);
-int wait_pthread(ButexPthreadWaiter& pw, timespec* ptimeout) {
+int wait_pthread(ButexPthreadWaiter& pw, const timespec* abstime) {
+ timespec * ptimeout = NULL;
+ timespec timeout;
+ int64_t timeout_us = 0;
+ int rc;
+
while (true) {
- const int rc = futex_wait_private(&pw.sig, PTHREAD_NOT_SIGNALLED, ptimeout);
- if (PTHREAD_NOT_SIGNALLED != pw.sig.load(butil::memory_order_acquire)) {
- // If `sig' is changed, wakeup_pthread() must be called and `pw'
- // is already removed from the butex.
- // Acquire fence makes this thread sees changes before wakeup.
- return rc;
+ if (abstime != NULL) {
+ timeout_us = butil::timespec_to_microseconds(*abstime) - butil::gettimeofday_us();
+ timeout = butil::microseconds_to_timespec(timeout_us);
+ ptimeout = &timeout;
}
- if (rc != 0 && errno == ETIMEDOUT) {
- // Note that we don't handle the EINTR from futex_wait here since
- // pthreads waiting on a butex should behave similarly as bthreads
- // which are not able to be woken-up by signals.
- // EINTR on butex is only producible by TaskGroup::interrupt().
-
- // `pw' is still in the queue, remove it.
+ if (timeout_us > MIN_SLEEP_US || abstime == NULL) {
+ rc = futex_wait_private(&pw.sig, PTHREAD_NOT_SIGNALLED, ptimeout);
+ if (PTHREAD_NOT_SIGNALLED != pw.sig.load(butil::memory_order_acquire)) {
+ // If `sig' is changed, wakeup_pthread() must be called and `pw'
+ // is already removed from the butex.
+ // Acquire fence makes this thread sees changes before wakeup.
+ return rc;
+ }
+ }
+ else {
Review Comment:
else不用换行
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For additional commands, e-mail: dev-help@brpc.apache.org
Re: [PR] Fix butex wait_pthread handle EINTR (brpc)
Posted by "chenBright (via GitHub)" <gi...@apache.org>.
chenBright commented on PR #2086:
URL: https://github.com/apache/brpc/pull/2086#issuecomment-1534013342
> @chenBright 可以帮忙重跑下clang-unittest吗~ 我看了失败的SocketTest.keepalive_input_message的逻辑跟butex无关，本地重跑也是可以通过的，怀疑是这个test case不稳定
done
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For additional commands, e-mail: dev-help@brpc.apache.org
[GitHub] [brpc] wasphin commented on a diff in pull request #2086: Fix butex wait_pthread handle EINTR
Posted by "wasphin (via GitHub)" <gi...@apache.org>.
wasphin commented on code in PR #2086:
URL: https://github.com/apache/brpc/pull/2086#discussion_r1084718111
##########
src/bthread/butex.cpp:
##########
@@ -137,27 +137,45 @@ static void wakeup_pthread(ButexPthreadWaiter* pw) {
bool erase_from_butex(ButexWaiter*, bool, WaiterState);
-int wait_pthread(ButexPthreadWaiter& pw, timespec* ptimeout) {
+int wait_pthread(ButexPthreadWaiter& pw, const timespec* abstime) {
+ timespec * ptimeout = NULL;
Review Comment:
`*` 靠左
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For additional commands, e-mail: dev-help@brpc.apache.org
[GitHub] [brpc] jamesge commented on a diff in pull request #2086: Fix butex wait_pthread handle EINTR
Posted by "jamesge (via GitHub)" <gi...@apache.org>.
jamesge commented on code in PR #2086:
URL: https://github.com/apache/brpc/pull/2086#discussion_r1086221251
##########
src/bthread/butex.cpp:
##########
@@ -137,27 +137,41 @@ static void wakeup_pthread(ButexPthreadWaiter* pw) {
bool erase_from_butex(ButexWaiter*, bool, WaiterState);
-int wait_pthread(ButexPthreadWaiter& pw, timespec* ptimeout) {
+int wait_pthread(ButexPthreadWaiter& pw, const timespec* abstime) {
+ timespec* ptimeout = NULL;
+ timespec timeout;
+ int64_t timeout_us = 0;
+ int rc;
+
while (true) {
- const int rc = futex_wait_private(&pw.sig, PTHREAD_NOT_SIGNALLED, ptimeout);
- if (PTHREAD_NOT_SIGNALLED != pw.sig.load(butil::memory_order_acquire)) {
- // If `sig' is changed, wakeup_pthread() must be called and `pw'
- // is already removed from the butex.
- // Acquire fence makes this thread sees changes before wakeup.
- return rc;
+ if (abstime != NULL) {
+ timeout_us = butil::timespec_to_microseconds(*abstime) - butil::gettimeofday_us();
+ timeout = butil::microseconds_to_timespec(timeout_us);
Review Comment:
这个timeout在下文中有用到么？
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For additional commands, e-mail: dev-help@brpc.apache.org
Re: [PR] Fix butex wait_pthread handle EINTR (brpc)
Posted by "jenrryyou (via GitHub)" <gi...@apache.org>.
jenrryyou commented on PR #2086:
URL: https://github.com/apache/brpc/pull/2086#issuecomment-1533960707
@chenBright 可以帮忙重跑下clang-unittest吗~ 我看了失败的SocketTest.keepalive_input_message的逻辑跟butex无关，本地重跑也是可以通过的，怀疑是这个test case不稳定
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For additional commands, e-mail: dev-help@brpc.apache.org
[GitHub] [brpc] jenrryyou commented on pull request #2086: Fix butex wait_pthread handle EINTR
Posted by "jenrryyou (via GitHub)" <gi...@apache.org>.
jenrryyou commented on PR #2086:
URL: https://github.com/apache/brpc/pull/2086#issuecomment-1407282216
> 能为这种情况增加一个单测么？（bthread的修改可能影响面很大）
可以的，最近有点忙，我晚点加下
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: devnull-unsubscribe@infra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix butex wait_pthread handle EINTR (brpc)
Posted by "wwbmmm (via GitHub)" <gi...@apache.org>.
wwbmmm merged PR #2086:
URL: https://github.com/apache/brpc/pull/2086
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For additional commands, e-mail: dev-help@brpc.apache.org
[GitHub] [brpc] wasphin commented on a diff in pull request #2086: Fix butex wait_pthread handle EINTR
Posted by "wasphin (via GitHub)" <gi...@apache.org>.
wasphin commented on code in PR #2086:
URL: https://github.com/apache/brpc/pull/2086#discussion_r1084718111
##########
src/bthread/butex.cpp:
##########
@@ -137,27 +137,45 @@ static void wakeup_pthread(ButexPthreadWaiter* pw) {
bool erase_from_butex(ButexWaiter*, bool, WaiterState);
-int wait_pthread(ButexPthreadWaiter& pw, timespec* ptimeout) {
+int wait_pthread(ButexPthreadWaiter& pw, const timespec* abstime) {
+ timespec * ptimeout = NULL;
Review Comment:
* 靠左
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For additional commands, e-mail: dev-help@brpc.apache.org
[GitHub] [brpc] jenrryyou commented on a diff in pull request #2086: Fix butex wait_pthread handle EINTR
Posted by "jenrryyou (via GitHub)" <gi...@apache.org>.
jenrryyou commented on code in PR #2086:
URL: https://github.com/apache/brpc/pull/2086#discussion_r1084834042
##########
src/bthread/butex.cpp:
##########
@@ -137,27 +137,45 @@ static void wakeup_pthread(ButexPthreadWaiter* pw) {
bool erase_from_butex(ButexWaiter*, bool, WaiterState);
-int wait_pthread(ButexPthreadWaiter& pw, timespec* ptimeout) {
+int wait_pthread(ButexPthreadWaiter& pw, const timespec* abstime) {
+ timespec * ptimeout = NULL;
+ timespec timeout;
+ int64_t timeout_us = 0;
+ int rc;
+
while (true) {
- const int rc = futex_wait_private(&pw.sig, PTHREAD_NOT_SIGNALLED, ptimeout);
- if (PTHREAD_NOT_SIGNALLED != pw.sig.load(butil::memory_order_acquire)) {
- // If `sig' is changed, wakeup_pthread() must be called and `pw'
- // is already removed from the butex.
- // Acquire fence makes this thread sees changes before wakeup.
- return rc;
+ if (abstime != NULL) {
+ timeout_us = butil::timespec_to_microseconds(*abstime) - butil::gettimeofday_us();
+ timeout = butil::microseconds_to_timespec(timeout_us);
+ ptimeout = &timeout;
}
- if (rc != 0 && errno == ETIMEDOUT) {
- // Note that we don't handle the EINTR from futex_wait here since
- // pthreads waiting on a butex should behave similarly as bthreads
- // which are not able to be woken-up by signals.
- // EINTR on butex is only producible by TaskGroup::interrupt().
-
- // `pw' is still in the queue, remove it.
+ if (timeout_us > MIN_SLEEP_US || abstime == NULL) {
+ rc = futex_wait_private(&pw.sig, PTHREAD_NOT_SIGNALLED, ptimeout);
+ if (PTHREAD_NOT_SIGNALLED != pw.sig.load(butil::memory_order_acquire)) {
+ // If `sig' is changed, wakeup_pthread() must be called and `pw'
+ // is already removed from the butex.
+ // Acquire fence makes this thread sees changes before wakeup.
+ return rc;
+ }
+ }
+ else {
Review Comment:
OK
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For additional commands, e-mail: dev-help@brpc.apache.org