base/synchronization/condition_variable_win.cc - Issue 2053403002: Fix Windows Condition Variable GetLastError Check

Unified Diff: base/synchronization/condition_variable_win.cc

Issue 2053403002: Fix Windows Condition Variable GetLastError Check (Closed) Base URL: https://chromium.googlesource.com/chromium/src.git@master

Patch Set: Created 4 years, 6 months ago

Use n/p to move between diff chunks; N/P to move between comments. Draft comments are only viewable by you.

Jump to:

View side-by-side diff with in-line comments

Download patch

Index: base/synchronization/condition_variable_win.cc

diff --git a/base/synchronization/condition_variable_win.cc b/base/synchronization/condition_variable_win.cc

index 61c6a715e0eccd8557338193f7f8313ab431b9ce..d5f24e4438753471dc38b0e48a0ae680477eafb2 100644

--- a/base/synchronization/condition_variable_win.cc

+++ b/base/synchronization/condition_variable_win.cc

@@ -34,8 +34,8 @@ void ConditionVariable::TimedWait(const TimeDelta& max_time) {

user_lock_->CheckHeldAndUnmark();

#endif

- if (FALSE == SleepConditionVariableSRW(&cv_, srwlock_, timeout, 0)) {

- DCHECK(GetLastError() != WAIT_TIMEOUT);

+ if (!SleepConditionVariableSRW(&cv_, srwlock_, timeout, 0)) {

+ DCHECK_EQ(static_cast<DWORD>(ERROR_TIMEOUT), GetLastError());

danakj 2016/06/10 17:37:11 What is the intention of the DCHECK before? It was

robliao 2016/06/10 18:16:12 My interpretation of the DCHECK is that it was a m

danakj 2016/06/10 21:00:09 Ah ok, there are in fact other errors possible. It

Ah ok, there are in fact other errors possible. It's clear that TIMEOUT is possible, so != TIMEOUT wouldn't make sense. LGTM

sky 2016/09/14 16:35:16 This is a great description of why the DCHECK shou

This is a great description of why the DCHECK should be the way you have it here! As the DCHECK isn't obvious I would add this comment to the code (which would have cut down on the review back and forth too).

robliao 2016/09/14 17:49:58 https://codereview.chromium.org/2341773003/ opened

On 2016/09/14 16:35:16, sky wrote: > On 2016/06/10 18:16:12, robliao wrote: > > On 2016/06/10 17:37:11, danakj wrote: > > > What is the intention of the DCHECK before? It was checking != WAIT_TIMEOUT. > > Did > > > it mean that failures should *not* be timeout? Now the DCHECK would verify > > they > > > are only timeout. > > > > > > https://msdn.microsoft.com/en-us/library/windows/desktop/ms686304(v=vs.85).aspx > > > doesn't mention other errors, so I'm not clear what the reason for the > DCHECK > > is > > > at all either. > > > > My interpretation of the DCHECK is that it was a misreading of the API spec. > > GetLastError() can never equal WAIT_TIMEOUT but it could equal ERROR_TIMEOUT. > > The intention is similar to POSIX, to make sure that the CV is either signaled > > (returns TRUE) or times out (returns FALSE, GetLastError() == ERROR_TIMEOUT). > > Any other case is unexpected and probably bad. > > > > If SleepConditionVariable(CS|SRW) returns FALSE, it means one of two things: > > 1) The CV timed out, which means GetLastError() == ERROR_TIMEOUT > > 2) The CV failed to sleep, which means GetLastError() == something else. > > > > From the linked article for SRW specifically: > > > Return value > > > > > If the function succeeds, the return value is nonzero. > > > If the function fails, the return value is zero. To get extended error > > information, call GetLastError. > > > If the timeout expires the function returns FALSE and GetLastError returns > > ERROR_TIMEOUT. > > > > CritSecs are worded differently with the same, if not clearer, effect. > > >Return value > > > > > >If the function succeeds, the return value is nonzero. > > >If the function fails or the time-out interval elapses, the return value is > > zero. To get extended error information, call GetLastError. Possible error > codes > > include ERROR_TIMEOUT, which indicates that the time-out interval has elapsed > > before another thread has attempted to wake the sleeping thread. > > This is a great description of why the DCHECK should be the way you have it > here! As the DCHECK isn't obvious I would add this comment to the code (which > would have cut down on the review back and forth too).

https://codereview.chromium.org/2341773003/ opened to add comments. My only hesitation on the windows-specific portion is if this was written correctly in the first place, this change wouldn't exist and no one would be the wiser. The casual Windows developer doing an audit would check this against MSDN and note the issue.

}

#if DCHECK_IS_ON()

« no previous file with comments | « no previous file | no next file » | no next file with comments »