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

Side by Side 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 unified diff | Download patch

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

OLD	NEW
1 // Copyright (c) 2011 The Chromium Authors. All rights reserved.	1 // Copyright (c) 2011 The Chromium Authors. All rights reserved.

2 // Use of this source code is governed by a BSD-style license that can be	2 // Use of this source code is governed by a BSD-style license that can be

3 // found in the LICENSE file.	3 // found in the LICENSE file.

4	4

5 #include "base/synchronization/condition_variable.h"	5 #include "base/synchronization/condition_variable.h"

6	6

7 #include "base/synchronization/lock.h"	7 #include "base/synchronization/lock.h"

8 #include "base/threading/thread_restrictions.h"	8 #include "base/threading/thread_restrictions.h"

9 #include "base/time/time.h"	9 #include "base/time/time.h"

10	10

(...skipping 16 matching lines...) Expand all Loading...
27 }	27 }

28	28

29 void ConditionVariable::TimedWait(const TimeDelta& max_time) {	29 void ConditionVariable::TimedWait(const TimeDelta& max_time) {

30 base::ThreadRestrictions::AssertWaitAllowed();	30 base::ThreadRestrictions::AssertWaitAllowed();

31 DWORD timeout = static_cast<DWORD>(max_time.InMilliseconds());	31 DWORD timeout = static_cast<DWORD>(max_time.InMilliseconds());

32	32

33 #if DCHECK_IS_ON()	33 #if DCHECK_IS_ON()

34 user_lock_->CheckHeldAndUnmark();	34 user_lock_->CheckHeldAndUnmark();

35 #endif	35 #endif

36	36

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

38 DCHECK(GetLastError() != WAIT_TIMEOUT);	38 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 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. robliao 2016/06/10 18:16:12 My interpretation of the DCHECK is that it was a m Show quoted text 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: Show quoted text > 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. Show quoted text >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. danakj 2016/06/10 21:00:09 Ah ok, there are in fact other errors possible. It Show quoted text 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. 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 Show quoted text 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). robliao 2016/09/14 17:49:58 https://codereview.chromium.org/2341773003/ opened Show quoted text 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.
39 }	39 }

40	40

41 #if DCHECK_IS_ON()	41 #if DCHECK_IS_ON()

42 user_lock_->CheckUnheldAndMark();	42 user_lock_->CheckUnheldAndMark();

43 #endif	43 #endif

44 }	44 }

45	45

46 void ConditionVariable::Broadcast() {	46 void ConditionVariable::Broadcast() {

47 WakeAllConditionVariable(&cv_);	47 WakeAllConditionVariable(&cv_);

48 }	48 }

49	49

50 void ConditionVariable::Signal() {	50 void ConditionVariable::Signal() {

51 WakeConditionVariable(&cv_);	51 WakeConditionVariable(&cv_);

52 }	52 }

53	53

54 } // namespace base	54 } // namespace base

OLD	NEW