Semaphore now able to be interrupted #380
Conversation
…al interrupt Signed-off-by: Coldwings <[email protected]>
thread/thread.h
Outdated
| @@ -362,10 +362,30 @@ namespace photon | |||
| { | |||
| return waitq::wait(timeout); | |||
| } | |||
| template<typename LOCK, typename PRED> | |||
| int wait_pred(LOCK& lock, PRED&& pred, Timeout timeout = {}) { | |||
There was a problem hiding this comment.
change to wait by using SFINAE
thread/thread.h
Outdated
| return do_wait_pred([&]{ return wait(lock, timeout); }, std::forward<PRED>(pred), timeout); | ||
| } | ||
| template<typename PRED> | ||
| int wait_pred_no_lock(PRED&& pred, Timeout timeout = {}) { |
There was a problem hiding this comment.
simply name it as wait_no_lock?
There was a problem hiding this comment.
change to wait_no_lock by using SFINAE
| } | ||
| errno = err; | ||
| return ret; | ||
| } | ||
| }; | ||
|
|
||
| class semaphore : protected waitq |
There was a problem hiding this comment.
add a wait_uninterruptible()?
Signed-off-by: Coldwings <[email protected]>
| thread* resume_one(int error_number = ECANCELED); | ||
| void resume(thread* th, int error_number = -1); // `th` must be waiting in this waitq! | ||
| int resume_all(int error_number = -1); | ||
| thread* resume_one(int error_number = -1); | ||
| waitq() = default; |
There was a problem hiding this comment.
What is the default behavior of the default value -1 ? Documents should be adjusted in:
https://github.com/alibaba/PhotonLibOS/blob/main/doc/docs/api/thread.md#thread_interrupt
There was a problem hiding this comment.
If it comes from the inside of the synchronization primitives due to technical reasons, we use -1 to distinguish from user generated ordinary error numbers (>0). And 0 has already been used for another scenario.
There was a problem hiding this comment.
What is the default behavior of the default value -1 ? Documents should be adjusted in:
https://github.com/alibaba/PhotonLibOS/blob/main/doc/docs/api/thread.md#thread_interrupt
Those documents are about photon::thread_interrupt. The default errno in photon::thread_interrupt is not changed, so it still EINTR, nothing has changed in document.
Here is all about method waitq::resume, which is protected method, not a part of API.
thread/thread.h
Outdated
| @@ -362,17 +362,52 @@ namespace photon | |||
| { | |||
| return waitq::wait(timeout); | |||
| } | |||
| template <typename LOCK, typename PRED, | |||
| typename = typename std::enable_if<std::is_same< | |||
| decltype(std::declval<PRED>()()), bool>::value>::type> | |||
There was a problem hiding this comment.
as long as PRED is not typed as Timeout
There was a problem hiding this comment.
and enable_if_t is shorter
There was a problem hiding this comment.
if do not limit PRED return value type, even enable_if part is meaningless.
Once PRED is callable (means PRED()() has a type) is good enough.
thread/thread.h
Outdated
| } | ||
| errno = err; | ||
| return ret; | ||
| } | ||
| }; | ||
|
|
||
| class semaphore : protected waitq | ||
| { | ||
| public: | ||
| explicit semaphore(uint64_t count = 0) : m_count(count) { } | ||
| int wait(uint64_t count, Timeout timeout = {}); |
There was a problem hiding this comment.
Is there any invocations of wait() that should be changed to invocations of wait_uninterruptible()?
There was a problem hiding this comment.
Make wait uninterruptible, keeps it works like it used to be, and add a new method wait_interruptible to present a wait that able to be interrupt might be better.
Signed-off-by: Coldwings <[email protected]>
This pull request improves the synchronization components based on
waitqby adding support for interruption usingthread_interrupt. The semaphore component, which was previously unable to be interrupted, is now included in this enhancement.Interruption of a running photon thread can be beneficial in specific situations, such as during the graceful shutdown of a process. This update enables more efficient and controlled termination of threads, resulting in improved system reliability and performance.
Additionally, the internal photon thread interruption errno used by
waitqhas been changed to -1 by default. As a result, users can now utilize all generic error numbers to interrupt synchronization components.