Re: [Development] Default enabling inbound flow control on sockets
-Original Message- From: development-bounces+shane.kearns=accenture@qt-project.org [mailto:development-bounces+shane.kearns=accenture@qt-project.org] On Behalf Of Thiago Macieira Sent: 13 February 2012 19:27 To: development@qt-project.org Subject: Re: [Development] Default enabling inbound flow control on sockets As you said yourself, there's a huge potential for regressions here, especially with a limit as low as 64k. So I am unsure whether we should do this at all. We could apply a high buffer size, like 1 MB. That's probably enough to contain most data files being downloaded and yet avoid exploding by OOM. On a modern embedded system, an application would need to be downloading 50 items in parallel (which means from at least 9 different servers) before it started to get into trouble. I'm also not convinced we should do this yet, but some more thoughts on the subject. When using the high level (QNetworkReply) API, you are dealing with objects. Intuitively, downloading a 100MB file will allocate 100MB of memory unless you take steps to prevent this. You might get more or less data than you were expecting, if the object on the server has changed. Regressions are likely, because downloading a complete object and then processing it would be a common usage. When using the low level (QTcpSocket) API, you are dealing with a stream. Intuitively, you would expect the stream applies flow control and rate limiting by itself. If using native apis (either synchronous or asynchronous) on all platforms, this is what happens. The exact amount of buffering in the OS level varies between platforms. With QTcpSocket, this does not happen, because Qt reads all available data from the OS into its own buffers each time the event loop runs socket notifiers. Regressions are less likely, because socket using code is normally processing the stream as it arrives. Subject to local law, communications with Accenture and its affiliates including telephone calls and emails (including content), may be monitored by our systems for the purposes of security and the assessment of internal compliance with Accenture policy. __ www.accenture.com ___ Development mailing list Development@qt-project.org http://lists.qt-project.org/mailman/listinfo/development
Re: [Development] Default enabling inbound flow control on sockets
On terça-feira, 14 de fevereiro de 2012 17.51.33, shane.kea...@accenture.com
wrote:
When using the low level (QTcpSocket) API, you are dealing with a stream.
Intuitively, you would expect the stream applies flow control and rate
limiting by itself. If using native apis (either synchronous or
asynchronous) on all platforms, this is what happens. The exact amount of
buffering in the OS level varies between platforms. With QTcpSocket, this
does not happen, because Qt reads all available data from the OS into its
own buffers each time the event loop runs socket notifiers. Regressions are
less likely, because socket using code is normally processing the stream as
it arrives.
Agreed. I think this applies more to QTcpSocket than to QNetworkReply.
But I'd still use a higher value for the buffer size. The kernel buffer size on
Windows is 48k and on Linux it can reach 128k. Also note the very common way
that people use QDataStream over QTcpSocket, which is:
void Class::readyReadSlot()
{
QDataStream stream(this);
if (size == -1 bytesAvailable() = sizeof(size))
stream buffer;
if (size bytesAvailable())
return;
// read using stream here
}
So the question is: what type is most recommended for the size variable?
What's the average size of the datastream packet?
To be honest, I'd also like to replace QDataStream with stream that did that
understood incomplete data (like QXmlStreamReader does).
--
Thiago Macieira - thiago.macieira (AT) intel.com
Software Architect - Intel Open Source Technology Center
Intel Sweden AB - Registration Number: 556189-6027
Knarrarnäsgatan 15, 164 40 Kista, Stockholm, Sweden
signature.asc
Description: This is a digitally signed message part.
___
Development mailing list
Development@qt-project.org
http://lists.qt-project.org/mailman/listinfo/development
Re: [Development] Default enabling inbound flow control on sockets
You've correctly understood the problem, but the scale is less bad (takes a couple of minutes to exhaust 2GB limit of a 32 bit process using a loopback connection) Changing the default would mean behavior is the same as if you called setReadBufferSize(65536) in the application. Currently the default is setReadBufferSize(0), where 0 has the special meaning of unlimited. Performance tests should not break, because they are reading as fast as possible. There may be a degradation on one side if the writer is faster than the reader. UDP sockets losing data if buffering is exceeded is acceptable (it's supposed to be an unreliable service) I hadn't thought too much about UDP at this point, just TCP. The readAll issue is with QNetworkReply rather than QTcpSocket. There are two programming models in use for this: Simple model: 1. send request 2. wait for the finished signal from the reply 3. call readAll to get the data Good model: 1. send request 2. each time the reply emits bytesAvailable 2a. call readAll to get the currently buffered data 2b. save / process currently buffered data 3. when finished signal is received, call readAll a final time to get the tail The simple model tends to be used where the expected data is small. The good model tends to be used where the data size is unknown (e.g. browsers, download managers) We could give a much larger buffer size for QNetworkReply (e.g. 1MB) to catch reasonable small downloads, but this might be harder for developers (as they only see the buffer limit rarely) Yes, QIODevice allows partial writes, but Q*Socket always accepts all the data in the current implementation. I actually think this side is less of a problem, because the developer knows what kind of data they will be sending. Therefore they are in the best position to decide if they need write flow control or not -Original Message- From: andrew.stanley-jo...@nokia.com [mailto:andrew.stanley-jo...@nokia.com] Sent: 11 February 2012 11:36 To: Kearns, Shane Subject: Re: [Development] Default enabling inbound flow control on sockets Ok, let me verify I understand the problem: 1. If a Qt app is not processing the data, Sockets continue reading no mater what. 2. This results in no flow control, and on a 10gigE connection results in the Qt app potentially receiving ~1 gigabyte/sec stream of data. (assuming the HW/SW etc can handle it) 3. Qt app exhausts all memory within a couple of seconds. This seems like relatively brain dead behaviour. On large systems it's a problem, and on smaller devices outright dangerous. What will changing the default buffer size do? I assume you mean set a reasonable default to QAbstractSocket::readBufferSize(). This seems sensible, it will stop the stream and use appropriate flow control, if available with that protocol. 1. Break performance tests. 2. datagram (UDP) sockets will lose data. 3. As you said, readAll() may have problems, but it's also incompatible with sane network programming. How can I trust the other side is only going to send 50 bytes and not 4 gigs? There's no requirement for write buffering as well. Under Unix's can you can write data to a socket and have it not block and/or have it write less data than available. QIODevice accounts for this and write can return 0 bytes written. You can easily argue most programs don't handle this well. But at some point network links can't handle traffic at any arbitrary rate and flow control must kick in. Buffering is a short term solution. I think it's a good idea to have a build in limit. I think it avoids easily made mistake by developers. -Andrew On 10/02/2012, at 8:25 PM, ext shane.kea...@accenture.com wrote: The current default behaviour of Qt sockets is that they allocate an unbounded amount of memory if the application is not reading all data from the socket but the event loop is running. In the worst case, this causes memory allocation failure resulting in a crash or app exit due to bad_alloc exception We could change the default read buffer size from 0 (unbounded) to a sensible default value e.g. 64k (to be benchmarked) Applications wanting the old behaviour could explicitly call setReadBufferSize(0) Applications that already set a read buffer size will be unchanged The same change would be required at the QNetworkAccessManager level, as there is no point applying flow control at the socket and having unbounded memory allocations in the QNetworkReply. This is a behavioural change that would certainly cause regressions. e.g. any application that waits for the QNetworkReply::finished() signal and then calls readAll() would break if the size of the object being downloaded is larger than the buffer size. On the other hand, we can't enable default outbound flow control in sockets (we don't want write to block). Applications need to use the bytesWritten signal. QNetworkAccessManager already implements outbound flow
Re: [Development] Default enabling inbound flow control on sockets
On 02/13/2012 08:10 AM, ext shane.kea...@accenture.com wrote: Not sure. Is it a big problem? Or is it better to just continue as is, and let the applications that do have a problem set it to something reasonable to them instead? I'd probably suggest that we instead improve the output on that worst case failure to help devs fix the problems in their programs. $0.02 Ben qWarning allocates memory, so once the worst case happens, we can't give any output (unless we first handle the exceptions inside Q*Socket) It would be possible to print a warning when buffering exceeds some threshold we consider to be unreasonably large. We can also improve the documentation (it's not bad, but doesn't mention flow control explicitly) https://bugreports.qt-project.org/browse/QTBUG-14464 seems to be explained by missing flow control causing out of memory errors under load. Qt is not known for handling OOM cases, and we actually argue for the OS to handle it, and that the application (and its libraries) shouldn't have to think about memory not being available. (Too many error conditions and OOM handling all over the place to bring any real benefit for it anyways.) Here you are suggesting a behavior change to handle an OOM case, where the behavior will most likely cause valid applications to stop working, since they don't handle a (new and artificial) 64k in-buffer limit? Maybe we should rather allocate a buffer for qWarning output, allowing us to properly report an error condition before the application aborts? ^shrug^ but I rather not have applications start failing at random due to a failure to have a new limit. -- .marius ___ Development mailing list Development@qt-project.org http://lists.qt-project.org/mailman/listinfo/development
Re: [Development] Default enabling inbound flow control on sockets
On Monday 13 February 2012 18:33:51 marius.storm-ol...@nokia.com wrote: On 02/13/2012 08:10 AM, ext shane.kea...@accenture.com wrote: Not sure. Is it a big problem? Or is it better to just continue as is, and let the applications that do have a problem set it to something reasonable to them instead? I'd probably suggest that we instead improve the output on that worst case failure to help devs fix the problems in their programs. $0.02 Ben qWarning allocates memory, so once the worst case happens, we can't give any output (unless we first handle the exceptions inside Q*Socket) It would be possible to print a warning when buffering exceeds some threshold we consider to be unreasonably large. We can also improve the documentation (it's not bad, but doesn't mention flow control explicitly) https://bugreports.qt-project.org/browse/QTBUG-14464 seems to be explained by missing flow control causing out of memory errors under load. Qt is not known for handling OOM cases, and we actually argue for the OS to handle it, and that the application (and its libraries) shouldn't have to think about memory not being available. (Too many error conditions and OOM handling all over the place to bring any real benefit for it anyways.) Here you are suggesting a behavior change to handle an OOM case, where the behavior will most likely cause valid applications to stop working, since they don't handle a (new and artificial) 64k in-buffer limit? Maybe we should rather allocate a buffer for qWarning output, allowing us to properly report an error condition before the application aborts? ^shrug^ but I rather not have applications start failing at random due to a failure to have a new limit. Here the problem is not really OOM, but bad putting way too much data structure into memory. QByteArray is not designed to contain huge data (2GiB is really the maximum, only because we use int for indexes) and stuff like readAll() will crash if that is hapenning. The second problem seems to be that QTCPSocket buffers everything, it does not matters if the program does anything with it or just let the event loop spin. If QTCPSocket did not buffer, the OS would buffer a bit, then drop the packet, so sender's TCP algorithms would reduce the rate of sending letting the time for the application to process. This is my naïve interpretation of the problem and I might be wrong here. But I think, indeed, the network and IO stack should do something against that. ___ Development mailing list Development@qt-project.org http://lists.qt-project.org/mailman/listinfo/development
Re: [Development] Default enabling inbound flow control on sockets
On 02/13/2012 12:33 PM, ext marius.storm-ol...@nokia.com wrote: ^shrug^ but I rather not have applications start failing at random due to a failure to have a new limit. ..handle a new limit. -- .marius ___ Development mailing list Development@qt-project.org http://lists.qt-project.org/mailman/listinfo/development
Re: [Development] Default enabling inbound flow control on sockets
On sexta-feira, 10 de fevereiro de 2012 19.25.04, shane.kea...@accenture.com wrote: We could change the default read buffer size from 0 (unbounded) to a sensible default value e.g. 64k (to be benchmarked) Applications wanting the old behaviour could explicitly call setReadBufferSize(0) Applications that already set a read buffer size will be unchanged [snip] This is a behavioural change that would certainly cause regressions. e.g. any application that waits for the QNetworkReply::finished() signal and then calls readAll() would break if the size of the object being downloaded is larger than the buffer size. As you said yourself, there's a huge potential for regressions here, especially with a limit as low as 64k. So I am unsure whether we should do this at all. We could apply a high buffer size, like 1 MB. That's probably enough to contain most data files being downloaded and yet avoid exploding by OOM. On a modern embedded system, an application would need to be downloading 50 items in parallel (which means from at least 9 different servers) before it started to get into trouble. -- Thiago Macieira - thiago.macieira (AT) intel.com Software Architect - Intel Open Source Technology Center Intel Sweden AB - Registration Number: 556189-6027 Knarrarnäsgatan 15, 164 40 Kista, Stockholm, Sweden signature.asc Description: This is a digitally signed message part. ___ Development mailing list Development@qt-project.org http://lists.qt-project.org/mailman/listinfo/development
[Development] Default enabling inbound flow control on sockets
The current default behaviour of Qt sockets is that they allocate an unbounded amount of memory if the application is not reading all data from the socket but the event loop is running. In the worst case, this causes memory allocation failure resulting in a crash or app exit due to bad_alloc exception We could change the default read buffer size from 0 (unbounded) to a sensible default value e.g. 64k (to be benchmarked) Applications wanting the old behaviour could explicitly call setReadBufferSize(0) Applications that already set a read buffer size will be unchanged The same change would be required at the QNetworkAccessManager level, as there is no point applying flow control at the socket and having unbounded memory allocations in the QNetworkReply. This is a behavioural change that would certainly cause regressions. e.g. any application that waits for the QNetworkReply::finished() signal and then calls readAll() would break if the size of the object being downloaded is larger than the buffer size. On the other hand, we can't enable default outbound flow control in sockets (we don't want write to block). Applications need to use the bytesWritten signal. QNetworkAccessManager already implements outbound flow control for uploads. Is making applications safe against this kind of overflow by default worth the compatibility breaks? Subject to local law, communications with Accenture and its affiliates including telephone calls and emails (including content), may be monitored by our systems for the purposes of security and the assessment of internal compliance with Accenture policy. __ www.accenture.com ___ Development mailing list Development@qt-project.org http://lists.qt-project.org/mailman/listinfo/development
Re: [Development] Default enabling inbound flow control on sockets
From: shane.kea...@accenture.com shane.kea...@accenture.com T he current default behaviour of Qt sockets is that they allocate an unbounded amount of memory if the application is not reading all data from the socket but the event loop is running. In the worst case, this causes memory allocation failure resulting in a crash or app exit due to bad_alloc exception We could change the default read buffer size from 0 (unbounded) to a sensible default value e.g. 64k (to be benchmarked) Applications wanting the old behaviour could explicitly call setReadBufferSize(0) Applications that already set a read buffer size will be unchanged The same change would be required at the QNetworkAccessManager level, as there is no point applying flow control at the socket and having unbounded memory allocations in the QNetworkReply. This is a behavioural change that would certainly cause regressions. e.g. any application that waits for the QNetworkReply::finished() signal and then calls readAll() would break if the size of the object being downloaded is larger than the buffer size. On the other hand, we can't enable default outbound flow control in sockets (we don't want write to block). Applications need to use the bytesWritten signal. QNetworkAccessManager already implements outbound flow control for uploads. Is making applications safe against this kind of overflow by default worth the compatibility breaks? Not sure. Is it a big problem? Or is it better to just continue as is, and let the applications that do have a problem set it to something reasonable to them instead? I'd probably suggest that we instead improve the output on that worst case failure to help devs fix the problems in their programs. $0.02 Ben ___ Development mailing list Development@qt-project.org http://lists.qt-project.org/mailman/listinfo/development
