[GitHub] [thrift] theilig commented on pull request #2134: THRIFT-5199: Fix infinite loop writing to closed TSocket in PHP

[GitBox <gi...@apache.org>]

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:
users@infra.apache.org