[
https://issues.apache.org/jira/browse/THRIFT-3791?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=15248720#comment-15248720
]
Kyle Johnson commented on THRIFT-3791:
--------------------------------------
The problem is that it isn't, at least with regard to ReadDirect and IsOpen.
If the extra IsOpen call is in there to ensure thread safety, it does no such
thing because Close could occur between the IsOpen check and the PeekNamedPipe
check, and even between PeekNamedPipe and ReadFile. It's just a race
condition. As such, it really doesn't matter whether or not IsOpen is called
once or multiple times within the loop. If thread safety is truly necessary,
then the following would be required:
1) Put the IsOpen check back into the loop.
2) Wrap all code between IsOpen and the API call using the handle with a
critical section. This means modifying ReadDirect and WriteDirect, both.
3) Wrap the Close() handler with a critical section.
However, if we want to just rely on Windows to return ERROR_INVALID_HANDLE
(which actually isn't guaranteed...Windows *could* reassign the handle to
another file), then fine. But as it is, before the patch above, the code isn't
thread safe with regard to the handle being modified by another thread via
Close between the IsOpen and Peek or the Peek and ReadFile.
> Delphi pipe client may fail even in a non-error condition
> ---------------------------------------------------------
>
> Key: THRIFT-3791
> URL: https://issues.apache.org/jira/browse/THRIFT-3791
> Project: Thrift
> Issue Type: Bug
> Components: Delphi - Library
> Affects Versions: 1.0
> Reporter: Kyle Johnson
> Assignee: Jens Geyer
> Fix For: 0.10.0
>
> Attachments:
> THRIFT-3791-fix-for-calling-GetLastError-even-on-success.patch
>
>
> In TPipeStreamBase.ReadDirect(), the code performs a peek on the pipe. If no
> data is available (bytes = 0), GetLastError is still checked, even though
> Microsoft documentation clearly states that not all functions set the last
> error code to 0 on success
> (https://msdn.microsoft.com/en-us/library/windows/desktop/ms679360(v=vs.85).aspx).
> Furthermore, because PeekNamedPipe only mentions that GetLastError should
> be called on failure, the logic of the if test must be changed.
--
This message was sent by Atlassian JIRA
(v6.3.4#6332)
