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/16 04:11:30 UTC
[GitHub] [brpc] wwbmmm commented on a diff in pull request #2086: Fix butex wait_pthread handle EINTR
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