[Qt-interest] A bug? Busy looping in QAbstractSocket triggered by full read buffer

[Thiago Macieira]

On Saturday, 11 de December de 2010 09:29:25 Brendon Higgins wrote:
> Hi list,
> 
> I think I might've found a bug, but I'm not familiar with code at this
> level, so I thought I'd better ask before making any accusations. :-)
> 
> In qabstractsocket.cpp, lines 1832--1859 are a "forever" loop, where the

Please include the Qt version number or SHA-1 of the commit you're looking at 
when you mention line numbers.

Lines 1832 through through 1836 for me are the comments just before the 
QAbstractSocket::waitForReadyRead function starts (which is line 1837). 
There's a "forever" on line 1862 for me and one on 1920, so I'll assume you 
meant the first one...

> code apparently busily waits for data to arrive ready to read. Notice
> lines 1849--1852. Let's assume readyToRead is true (also assume the socket

I guess you mean these lines:

  1879          if (readyToRead) {
  1880              if (d->canReadNotification())
  1881                  return true;
  1882          }

> is still connected). Line 1850 calls canReadNotification(), which (at line
> 608) returns false if the read buffer has no more space. So the busy loop
> will not exit at line 1851 like (presumably) it normally would. And since
> the socket is still connected, the loop does not terminate at line 1858,
> either (or at all). These conditions don't change---the buffer is not
> (cannot be) flushed by anything in this loop, AFAICT---so the loop never
> terminates (until the connection eventually times out, presumably).
> 
> I found this issue using KDE and downloading a large file from a nearby
> high-bandwidth server. The kio_http process stops downloading and starts
> using up 100%. Attaching gdb to the process, it appears to be busy looping
> in the aforementioned code.

Could be a bug...

> Seems to me maybe there should be some sort of error emitted if the buffer
> is overrun, but that apparently isn't happening.
> 
> So, is this a bug, or are there subtleties I'm missing?

It sounds like an unforeseen condition. If the buffer is full, QAbstractSocket 
cannot read more data. But that also means it can't emit waitForReadyRead, 
because it hasn't read more data.

So it sounds like it should return under those conditions with a new error 
code.

At the same time, the caller should avoid calling waitForReadyRead if the 
buffer is already full. It's asking for an impossible condition. If the timeout 
is not -1, we might code the function so that it simply sleeps for the timeout 
period.

-- 
Thiago Macieira - thiago (AT) macieira.info - thiago (AT) kde.org
  Senior Product Manager - Nokia, Qt Development Frameworks
      PGP/GPG: 0x6EF45358; fingerprint:
      E067 918B B660 DBD1 105C  966C 33F5 F005 6EF4 5358
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 190 bytes
Desc: This is a digitally signed message part.
Url : http://lists.qt-project.org/pipermail/qt-interest-old/attachments/20101211/9265e052/attachment.bin