Side Effects …

By Alexander Schmidt

Usually, I admire the clean structure and high quality of the WRK source code. However, I happened to encouter what I do not call a good programming style. Have a look at the following function, KiWaitTest (in base\ntos\ke):

00198 {
00199
00200     PKEVENT Event;
00201     PLIST_ENTRY ListHead;
00202     PRKTHREAD Thread;
00203     PRKWAIT_BLOCK WaitBlock;
00204     PLIST_ENTRY WaitEntry;
00205     NTSTATUS WaitStatus;
00206
00207     //
00208     // As long as the signal state of the specified object is Signaled and
00209     // there are waiters in the object wait list, then try to satisfy a wait.
00210     //
00211
00212     Event = (PKEVENT)Object;
00213     ListHead = &Event->Header.WaitListHead;
00214     WaitEntry = ListHead->Flink;
00215     while ((Event->Header.SignalState > 0) &&
00216            (WaitEntry != ListHead)) {
00217
00218         WaitBlock = CONTAINING_RECORD(WaitEntry, KWAIT_BLOCK, WaitListEntry);
00219         Thread = WaitBlock->Thread;
00220         WaitStatus = STATUS_KERNEL_APC;
00221
00222         //
00223         // N.B. The below code only satisfies the wait for wait any types.
00224         //      Wait all types are satisfied in the wait code itself. This
00225         //      is done with a eye to the future when the dispatcher lock is
00226         //      split into a lock per waitable object type and a scheduling
00227         //      state lock. For now, a kernel APC is simulated for wait all
00228         //      types.
00229         //
00230
00231         if (WaitBlock->WaitType == WaitAny) {
00232             WaitStatus = (NTSTATUS)WaitBlock->WaitKey;
00233             KiWaitSatisfyAny((PKMUTANT)Event, Thread);
00234         }
00235
00236         KiUnwaitThread(Thread, WaitStatus, Increment);
00237         WaitEntry = ListHead->Flink;
00238     }
00239
00240     return;
00241 }

The purpose of this function is as follows:

This function tests if a wait can be satisfied when an object attains a state of signaled. If a wait can be satisfied, then the subject thread is unwaited with a completion status that is the WaitKey of the wait block from the object wait list. As many waits as possible are satisfied.

Despite the interesting comment on line 225, I find the code rather poor with respect to maintainability. Have a closer look at the while loop. The loop continues as long as the object is in a signaled state and there is any thread in the wait list for this object. So far so good. Now look at the end of the loop, where WaitEntry is supposed to be set to the next entry in the wait list. However, the statement is exactly the same as in line 214. This seems a bit confusing as it suggests that the loop may loop forever. Well, apparently it does not but it seems unclear why the loop terminates at all.

My first idea was that one of the functions certainly will update the wait list as a side effect. Let's have a look at KiUnwaitThread.

00146 {
00147
00148     //
00149     // Unlink thread from the appropriate wait queues and set the wait
00150     // completion status.
00151     //
00152
00153     KiUnlinkThread(Thread, WaitStatus);
00154
00155     //
00156     // Set unwait priority adjustment parameters.
00157     //
00158
00159     ASSERT(Increment >= 0);
00160
00161     Thread->AdjustIncrement = (SCHAR)Increment;
00162     Thread->AdjustReason = (UCHAR)AdjustUnwait;
00163
00164     //
00165     // Ready the thread for execution.
00166     //
00167
00168     KiReadyThread(Thread);
00169     return;
00170 }

Again, at first sight, there are no modifications to any wait queues here. Given the parameters to the function, Thread, WaitStatus, and Increment, I wouldn't even expect any modification here. However, the KiUnlinkThread function finally reveals the "mystery".

03077     //
03078     // Set wait completion status, remove wait blocks from object wait
03079     // lists, and remove thread from wait list.
03080     //
03081
03082     Thread->WaitStatus |= WaitStatus;
03083     WaitBlock = Thread->WaitBlockList;
03084     do {
03085         RemoveEntryList(&WaitBlock->WaitListEntry);
03086         WaitBlock = WaitBlock->NextWaitBlock;
03087     } while (WaitBlock != Thread->WaitBlockList);

             // ...

The WaitBlockList of a thread comprises the set of objects the thread is waiting for. Part of the wait block is the WaitListEntry field. This field is used as linking element for the wait list that we traversed in the first function. Thus, removing the wait block here will implicitly update the wait list structure of any object the thread is waiting on.

I think that hiding this very detail behind two nested function calls can potentially lead to complications, especially when functions grow bigger and you have more than one person working on it. There might be few people out there, who can recall any nifty detail about their projects, but I doubt the majority of us can. Usually - and that's why I am a bit disappointed here - the WRK contains a comment in such situation pointing to the side effect, but here is none. So keep in mind the Zen of Python and try to avoid such things in your own implementations:

Side Effects …

Comments