Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2053403002:
Fix Windows Condition Variable GetLastError Check (Closed)
DescriptionFix Windows Condition Variable GetLastError Check
During the SleepConditionVariableCS days, the GetLastError was checked
against WAIT_TIMEOUT. This should have been ERROR_TIMEOUT per the docs.
WAIT_TIMEOUT applies to functions like WaitForSingleObject (and more).
BUG=619037
Committed: https://crrev.com/6162e2705ec3496cddb769e08c4c7fb24f7a85b3
Cr-Commit-Position: refs/heads/master@{#399279}
Patch Set 1 #
Total comments: 5
Messages
Total messages: 23 (10 generated)
Description was changed from ========== Fix Windows Condition Variable Return Value Check During the SleepConditionVariableCS days, the return value checked against WAIT_TIMEOUT. This should have been ERROR_TIMEOUT per the docs. WAIT_TIMEOUT applies to functions like WaitForSingleObject (and more). BUG=619037 ========== to ========== Fix Windows Condition Variable GetLastError Check During the SleepConditionVariableCS days, the GetLastError was checked against WAIT_TIMEOUT. This should have been ERROR_TIMEOUT per the docs. WAIT_TIMEOUT applies to functions like WaitForSingleObject (and more). BUG=619037 ==========
Description was changed from ========== Fix Windows Condition Variable GetLastError Check During the SleepConditionVariableCS days, the GetLastError was checked against WAIT_TIMEOUT. This should have been ERROR_TIMEOUT per the docs. WAIT_TIMEOUT applies to functions like WaitForSingleObject (and more). BUG=619037 ========== to ========== Fix Windows Condition Variable GetLastError Check During the SleepConditionVariableCS days, the GetLastError was checked against WAIT_TIMEOUT. This should have been ERROR_TIMEOUT per the docs. WAIT_TIMEOUT applies to functions like WaitForSingleObject (and more). BUG=619037 ==========
The CQ bit was checked by robliao@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2053403002/1
robliao@chromium.org changed reviewers: + danakj@chromium.org
prashant.n@samsung.com changed reviewers: + prashant.n@samsung.com
lgtm
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2053403002/diff/1/base/synchronization/condit... File base/synchronization/condition_variable_win.cc (right): https://codereview.chromium.org/2053403002/diff/1/base/synchronization/condit... base/synchronization/condition_variable_win.cc:38: DCHECK_EQ(static_cast<DWORD>(ERROR_TIMEOUT), GetLastError()); 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.
https://codereview.chromium.org/2053403002/diff/1/base/synchronization/condit... File base/synchronization/condition_variable_win.cc (right): https://codereview.chromium.org/2053403002/diff/1/base/synchronization/condit... base/synchronization/condition_variable_win.cc:38: DCHECK_EQ(static_cast<DWORD>(ERROR_TIMEOUT), GetLastError()); 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.
https://codereview.chromium.org/2053403002/diff/1/base/synchronization/condit... File base/synchronization/condition_variable_win.cc (right): https://codereview.chromium.org/2053403002/diff/1/base/synchronization/condit... base/synchronization/condition_variable_win.cc:38: DCHECK_EQ(static_cast<DWORD>(ERROR_TIMEOUT), GetLastError()); 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
The CQ bit was checked by robliao@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2053403002/1
Message was sent while issue was closed.
Description was changed from ========== Fix Windows Condition Variable GetLastError Check During the SleepConditionVariableCS days, the GetLastError was checked against WAIT_TIMEOUT. This should have been ERROR_TIMEOUT per the docs. WAIT_TIMEOUT applies to functions like WaitForSingleObject (and more). BUG=619037 ========== to ========== Fix Windows Condition Variable GetLastError Check During the SleepConditionVariableCS days, the GetLastError was checked against WAIT_TIMEOUT. This should have been ERROR_TIMEOUT per the docs. WAIT_TIMEOUT applies to functions like WaitForSingleObject (and more). BUG=619037 ==========
Message was sent while issue was closed.
Committed patchset #1 (id:1)
Message was sent while issue was closed.
CQ bit was unchecked
Message was sent while issue was closed.
Description was changed from ========== Fix Windows Condition Variable GetLastError Check During the SleepConditionVariableCS days, the GetLastError was checked against WAIT_TIMEOUT. This should have been ERROR_TIMEOUT per the docs. WAIT_TIMEOUT applies to functions like WaitForSingleObject (and more). BUG=619037 ========== to ========== Fix Windows Condition Variable GetLastError Check During the SleepConditionVariableCS days, the GetLastError was checked against WAIT_TIMEOUT. This should have been ERROR_TIMEOUT per the docs. WAIT_TIMEOUT applies to functions like WaitForSingleObject (and more). BUG=619037 Committed: https://crrev.com/6162e2705ec3496cddb769e08c4c7fb24f7a85b3 Cr-Commit-Position: refs/heads/master@{#399279} ==========
Message was sent while issue was closed.
Patchset 1 (id:??) landed as https://crrev.com/6162e2705ec3496cddb769e08c4c7fb24f7a85b3 Cr-Commit-Position: refs/heads/master@{#399279}
Message was sent while issue was closed.
sky@chromium.org changed reviewers: + sky@chromium.org
Message was sent while issue was closed.
Sorry for commenting on an old patch. https://codereview.chromium.org/2053403002/diff/1/base/synchronization/condit... File base/synchronization/condition_variable_win.cc (right): https://codereview.chromium.org/2053403002/diff/1/base/synchronization/condit... base/synchronization/condition_variable_win.cc:38: DCHECK_EQ(static_cast<DWORD>(ERROR_TIMEOUT), GetLastError()); 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).
Message was sent while issue was closed.
https://codereview.chromium.org/2053403002/diff/1/base/synchronization/condit... File base/synchronization/condition_variable_win.cc (right): https://codereview.chromium.org/2053403002/diff/1/base/synchronization/condit... base/synchronization/condition_variable_win.cc:38: DCHECK_EQ(static_cast<DWORD>(ERROR_TIMEOUT), GetLastError()); 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. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
