astitcher commented on a change in pull request #290:
URL: https://github.com/apache/qpid-proton/pull/290#discussion_r561995691
##########
File path: c/src/proactor/epoll-internal.h
##########
@@ -79,36 +79,36 @@ typedef enum {
LISTENER,
RAW_CONNECTION,
TIMER_MANAGER
-} pcontext_type_t;
+} task_type_t;
-typedef struct pcontext_t {
+typedef struct task_t {
pmutex mutex;
pn_proactor_t *proactor; /* Immutable */
- pcontext_type_t type;
+ task_type_t type;
bool working;
- bool on_wake_list;
- bool wake_pending; // unprocessed eventfd wake callback
- struct pcontext_t *wake_next; // wake list, guarded by proactor eventfd_mutex
+ bool on_ready_list;
+ bool ready; // ready to run and on ready list. Poller
notified by eventfd.
+ struct task_t *ready_next; // ready list, guarded by proactor eventfd_mutex
bool closing;
// Next 4 are protected by the proactor mutex
- struct pcontext_t* next; /* Protected by proactor.mutex */
- struct pcontext_t* prev; /* Protected by proactor.mutex */
+ struct task_t* next; /* Protected by proactor.mutex */
+ struct task_t* prev; /* Protected by proactor.mutex */
int disconnect_ops; /* ops remaining before disconnect complete */
bool disconnecting; /* pn_proactor_disconnect */
// Protected by schedule mutex
tslot_t *runner __attribute__((aligned(64))); /* designated or running
thread */
tslot_t *prev_runner;
- bool sched_wake;
+ bool sched_ready;
bool sched_pending; /* If true, one or more unseen epoll or other
events to process() */
- bool runnable ; /* in need of scheduling */
-} pcontext_t;
+ bool runnable ; /* on one of the runnable lists */
Review comment:
Looking at all the bools here, I wonder if there is actually a hidden
state machine that would make the task state more comprehensible and safer. I
expect that these bools aren't truly independent and so naming and coding out
the actual allowed states might make the lifecycle of tasks easier to
understand.
There might be some locking issues here as the bools seem to be covered by
different locks, but this seems to be a code smell in itself anyway as I'd
expect things covered by a lock to be in the same structure as the lock not
elsewhere!!
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
