theilig commented on pull request #2134:
URL: https://github.com/apache/thrift/pull/2134#issuecomment-626209869
I saw that comment on php.net and verified it was true (both by looking at
the c implementation of php's fwrite, and by writing a simple test.
I believe that checking for written <= 0 would work and would be more
robust. If there is a closed duplex socket where data was sent from the peer
before closing then fwrite will return 0 (the socket is closed) but feof will
return false because there is still data to read.
In the patch we are using locally I didn't go that route for two reasons:
1. I'm don't know enough about the internals of sockets to know if there are
some cases where a write of 0 is temporary and a subsequent write would be
successful.
2. The implementation of TSocket::read uses the 0 length + feof check so I
erred on the side of consistency.
`if ($readable > 0) {
$data = fread($this->handle_, $len);
if ($data === false) {
throw new TTransportException('TSocket: Could not read ' .
$len . ' bytes from ' .
$this->host_ . ':' . $this->port_);
} elseif ($data == '' && feof($this->handle_)) {
throw new TTransportException('TSocket read 0 bytes');
}
return $data;
}`
I'm pretty sure that throwing an exception on 0 bytes in both cases without
the feof check would be correct. We just decided to be really conservative
with our initial change as we make tens of thousands of thrift calls per second.
I can attest that adding the feof check has fixed our issue with stuck php
processes.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
[email protected]
