Re: [Development] API review for a new QDnsResolver class
- finalise the class name (I think QDns was suggested) Could we get this point settled once and for all? My suggestions: - QDnsLookup (my preferred one, as the object represents a lookup and its result) - QDnsInfo (similar to QHostInfo) - QDns Jeremy ___ Development mailing list Development@qt-project.org http://lists.qt-project.org/mailman/listinfo/development
Re: [Development] API review for a new QDnsResolver class
On Friday, 6 de January de 2012 18.46.16, Jeremy =?ISO-8859-1?Q?Lainé?wrote: - finalise the class name (I think QDns was suggested) Could we get this point settled once and for all? My suggestions: - QDnsLookup (my preferred one, as the object represents a lookup and its result) Go for it. - QDnsInfo (similar to QHostInfo) - QDns -- 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] API review for a new QDnsResolver class
Hi, On 05/01/2012, at 1:11 AM, ext Thiago Macieira wrote: On Thursday, 5 de January de 2012 12.07.37, craig.sc...@csiro.au wrote: On 05/01/2012, at 11:47 AM, Thiago Macieira wrote: On Thursday, 5 de January de 2012 11.03.42, craig.sc...@csiro.au wrote: This could be perceived as creating a race condition. You'd have to connect a slot to the signal on the object returned, but what if the signal is emitted before you get a chance to do that? It cannot happen if you make it, by design, not possible. That means the reply object must ensure that it never processes replies before first returning to the user. If a result was found cached (if ever there's a cache), then we need to be able to tell the user that the reply has already finished. Even if it never processes replies before first returning the user, there is the possibility of a race condition: No, there isn't. Your scenario below implies the user willfully connected late. All you need to do is connect the signal before returning to the event loop. That's how QNetworkReply ensures it works. You can avoid the signal-connection race condition like this, and it does work well in QNetworkReply. However, it is a little frustrating as a programmer that you have to connect signals and slots (not the fastest things in the world), and unnecessarily reenter the Qt event loop when you know that the data is often already available. If you expect the DNS lookups to be cached, perhaps you should follow the simple pattern that QDeclarativeComponent uses - where we have an isReady() method that you check, and only if that is false do you need to connect to the signal. Cheers, Aaron (1) Reply object is returned to the user. (2) Reply object processes the request and emits the relevant finished signal in its own thread. (3) Caller adds a signal-slot connection (could be on the line immediately following the function call that returned the reply object), but the signal has already been missed. -- 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 ___ Development mailing list Development@qt-project.org http://lists.qt-project.org/mailman/listinfo/development ___ Development mailing list Development@qt-project.org http://lists.qt-project.org/mailman/listinfo/development
Re: [Development] API review for a new QDnsResolver class
On 05/01/2012, at 3:51 AM, Jeremy Lainé wrote: Replying to myself to try and get the discussion going again. A summary of API decisions so far: - we only provide an asynchronous API - we do not want a manager object (QNAM-style) to avoid users creating a manager per lookup - we want a single class to support different types of DNS requests (Q3Dns-style) - lookups are performed by calling static methods returning a pointer to a QObject-derived class, which emits the finished() signal upon completion This could be perceived as creating a race condition. You'd have to connect a slot to the signal on the object returned, but what if the signal is emitted before you get a chance to do that? In case you think this is unlikely, we have seen issues like this when running code on virtual machines which seem to have markedly different inter-thread timing behaviour compared to physical machines. You can get longer delays on a thread than you might intuitively expect. What remains to be decided: - do we want shared pointers or not? My feeling is that using QSharedPointer is going to be awkward for the API user, I cannot find any other example of a Qt API which returns a QSharedPointer to a QObject-derived class. - finalise the class name (I think QDns was suggested) In the meantime, I have updated my implementation assuming we use raw pointers, with QDnsReply as the working name for the class: https://qt.gitorious.org/~sharky/qt/sharkys-qtbase/blobs/8628259746664df7754f7b27d406d7bbd74580b0/src/network/kernel/qdnsreply.h#line154 Some notes about the implementation: - the actual lookup code is run in a thread, managed by a global threadpool (QHostInfo-style) What happens if the client/user is also employing their own thread pool? Does your implementation simply set some arbitrary number of threads it will use and assume the system will take care of scheduling threads fairly (probably the only safe option)? Are you making use of QThread::idealThreadCount(), in which case it probably won't give the behaviour expected with the client/user also employing their own separate thread pool? Just wondering if using a thread pool here is really buying you anything over simply using QThread directly (but I guess if you are trying to manage supporting a large number of DNS queries then it makes sense to limit how many can be processed at once via a thread pool). - the thread and the QDnsReply both operate on a QSharedPointerQDnsReplyPrivate, so it doesn't matter whether the QDnsReply is deleted before or after the thread completes -- Dr Craig Scott Computational Software Engineering Team Leader, CSIRO (CMIS) Melbourne, Australia ___ Development mailing list Development@qt-project.org http://lists.qt-project.org/mailman/listinfo/development
Re: [Development] API review for a new QDnsResolver class
On Thursday, 5 de January de 2012 12.07.37, craig.sc...@csiro.au wrote: On 05/01/2012, at 11:47 AM, Thiago Macieira wrote: On Thursday, 5 de January de 2012 11.03.42, craig.sc...@csiro.au wrote: This could be perceived as creating a race condition. You'd have to connect a slot to the signal on the object returned, but what if the signal is emitted before you get a chance to do that? It cannot happen if you make it, by design, not possible. That means the reply object must ensure that it never processes replies before first returning to the user. If a result was found cached (if ever there's a cache), then we need to be able to tell the user that the reply has already finished. Even if it never processes replies before first returning the user, there is the possibility of a race condition: No, there isn't. Your scenario below implies the user willfully connected late. All you need to do is connect the signal before returning to the event loop. That's how QNetworkReply ensures it works. (1) Reply object is returned to the user. (2) Reply object processes the request and emits the relevant finished signal in its own thread. (3) Caller adds a signal-slot connection (could be on the line immediately following the function call that returned the reply object), but the signal has already been missed. -- 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] API review for a new QDnsResolver class
On 05/01/2012, at 12:11 PM, Thiago Macieira wrote: On Thursday, 5 de January de 2012 12.07.37, craig.sc...@csiro.au wrote: On 05/01/2012, at 11:47 AM, Thiago Macieira wrote: On Thursday, 5 de January de 2012 11.03.42, craig.sc...@csiro.au wrote: This could be perceived as creating a race condition. You'd have to connect a slot to the signal on the object returned, but what if the signal is emitted before you get a chance to do that? It cannot happen if you make it, by design, not possible. That means the reply object must ensure that it never processes replies before first returning to the user. If a result was found cached (if ever there's a cache), then we need to be able to tell the user that the reply has already finished. Even if it never processes replies before first returning the user, there is the possibility of a race condition: No, there isn't. Your scenario below implies the user willfully connected late. All you need to do is connect the signal before returning to the event loop. That's how QNetworkReply ensures it works. Ah, the reply object emits the signal in the same thread as the caller? I was assuming the signal is emitted from within the thread that is processing the reply. Yes, in that case connecting before returning to the event loop would be sufficient. (1) Reply object is returned to the user. (2) Reply object processes the request and emits the relevant finished signal in its own thread. (3) Caller adds a signal-slot connection (could be on the line immediately following the function call that returned the reply object), but the signal has already been missed. -- Dr Craig Scott Computational Software Engineering Team Leader, CSIRO (CMIS) Melbourne, Australia ___ Development mailing list Development@qt-project.org http://lists.qt-project.org/mailman/listinfo/development
Re: [Development] API review for a new QDnsResolver class
On Thursday, 5 de January de 2012 12.51.50, craig.sc...@csiro.au wrote: Ah, the reply object emits the signal in the same thread as the caller? I was assuming the signal is emitted from within the thread that is processing the reply. Yes, in that case connecting before returning to the event loop would be sufficient. The returned object lives in the calling thread, since that's required to be able to use the reply QObject in the first place. So it cannot be doing anything in calling thread since the user has control. Another thread may be working and may even emit the signal. But it will be queued, so it won't get processed until this thread goes back to the event loop. -- 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] API review for a new QDnsResolver class
On 01/05/2012 01:47 AM, Thiago Macieira wrote: On Thursday, 5 de January de 2012 11.03.42, craig.sc...@csiro.au wrote: This could be perceived as creating a race condition. You'd have to connect a slot to the signal on the object returned, but what if the signal is emitted before you get a chance to do that? It cannot happen if you make it, by design, not possible. That means the reply object must ensure that it never processes replies before first returning to the user. If a result was found cached (if ever there's a cache), then we need to be able to tell the user that the reply has already finished. As you stated, the reply object lives in the calling thread, and the finished() signal is always emitted in a queued fashion, so I think we're OK. Jeremy ___ Development mailing list Development@qt-project.org http://lists.qt-project.org/mailman/listinfo/development
Re: [Development] API review for a new QDnsResolver class
On 01/05/2012 01:03 AM, craig.sc...@csiro.au wrote: Some notes about the implementation: - the actual lookup code is run in a thread, managed by a global threadpool (QHostInfo-style) What happens if the client/user is also employing their own thread pool? Does your implementation simply set some arbitrary number of threads it will use and assume the system will take care of scheduling threads fairly (probably the only safe option)? Are you making use of QThread::idealThreadCount(), in which case it probably won't give the behaviour expected with the client/user also employing their own separate thread pool? Just wondering if using a thread pool here is really buying you anything over simply using QThread directly (but I guess if you are trying to manage supporting a large number of DNS queries then it makes sense to limit how many can be processed at once via a thread pool). I followed QHostInfo's pattern exactly here: I set a fixed maximum number of threads (5) on the pool. Jeremy ___ Development mailing list Development@qt-project.org http://lists.qt-project.org/mailman/listinfo/development
Re: [Development] API review for a new QDnsResolver class
On Thursday, 10 de November de 2011 07.54.55, Jeremy Lainé wrote: What I mean is that the Q3Dns has setters as well as a constructor with arguments, so for instance: Q3Dns *dns = new Q3Dns; dns-setLabel(foodomain.org);// will a request be triggered if I stop here and don't specify the record type? dns-setRecordType(Q3Dns::); You can remove this ambiguity by adding a lookup() slot function that executes the lookup, like QFile::open after QFile::setFileName. One problem with the API is the need to have separate processing of the components of a SRV label. -- Thiago Macieira - thiago (AT) macieira.info - thiago (AT) kde.org Software Architect - Intel Open Source Technology Center PGP/GPG: 0x6EF45358; fingerprint: E067 918B B660 DBD1 105C 966C 33F5 F005 6EF4 5358 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] API review for a new QDnsResolver class
On Wednesday, 9 de November de 2011 09:17:59 Jeremy Lainé wrote: On 11/08/2011 10:57 PM, Thiago Macieira wrote: On Tuesday, 8 de November de 2011 19:40:13 Jeremy Lainé wrote: - the QNAM-style API seems to be OK Correct, but all functions in QDnsResolver are static. That means they could go into QDnsReply and we could rename the class simply QDns. It worked for Qt 3... The methods are not static, the QDnsResolver instance initially owns all QDnsReply objects it returns. It also owns the QThreadPool used to perform the lookups. No need if all functions are static. And since they are not? They should be. There's no need for a QDnsResolver public object like QNetworkAccessManager. The idea of a manager in QNAM was to share settings, open connections and cookies. Usually, applications have only one QNAM and that's enough for them. However, in some circumstances, connections with different settings are necessary. Not so for DNS: the are no settings to be changed. So all queries use the same settings and resources, which are shared behind the scenes. All lookup functions should be static returning QDnsReply*, or it should be able to create the lookup using QDnsReply's constructor. -- 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] API review for a new QDnsResolver class
On Wed, Nov 9, 2011 at 9:17 AM, Jeremy Lainé jeremy.la...@m4x.org wrote: On 11/08/2011 10:57 PM, Thiago Macieira wrote: On Tuesday, 8 de November de 2011 19:40:13 Jeremy Lainé wrote: - the QNAM-style API seems to be OK Correct, but all functions in QDnsResolver are static. That means they could go into QDnsReply and we could rename the class simply QDns. It worked for Qt 3... The methods are not static, the QDnsResolver instance initially owns all QDnsReply objects it returns. It also owns the QThreadPool used to perform the lookups. Initially? What does that mean exactly? When does QDnsResolver stops owning the QDnsReply objects? - I have implemented QDnsReply::abort() to cancel a lookup request Good. I forgot to mention: you can delete the QDnsReply at any time, as both QDnsReply and QDnsResolverRunnable access QDnsReplyPrivate via a QSharedPointer. In that case, wouldn't it make more sense then to return a QDnsReply (so not a pointer)? Hmmm... but that is of course not possible because it derives from QObject, right? - Robin mentioned adding a static QDnsResolver::instance() method, does anyone else have an opinion on this? No need if all functions are static. And since they are not? As I argued before: I think they should be, as the class you showed up contains no actual data (from the user's perspective of it, anyway). It causes problems with having to keep the instance alive while the DNS request is running, even though the object itself can not really be interacted with anymore. It doesn't even provide API to know if it can be savely deleted, or if it is still in the background managing running requests. So, when can the API user savely get rid of the requester object? IMHO, it would make sense to have the methods on QDnsResolver be static, and let those static methods reference some private (singleton?) instance of the object that owns stuff like a threadpool and the QDnsReplies. That frees the user from having to care about the lifetime of the resolver object. Andre ___ Development mailing list Development@qt-project.org http://lists.qt-project.org/mailman/listinfo/development
Re: [Development] API review for a new QDnsResolver class
Le Nov 9, 2011 à 10:14 AM, André Somers a écrit : On Wed, Nov 9, 2011 at 9:17 AM, Jeremy Lainé jeremy.la...@m4x.org wrote: On 11/08/2011 10:57 PM, Thiago Macieira wrote: On Tuesday, 8 de November de 2011 19:40:13 Jeremy Lainé wrote: - the QNAM-style API seems to be OK Correct, but all functions in QDnsResolver are static. That means they could go into QDnsReply and we could rename the class simply QDns. It worked for Qt 3... The methods are not static, the QDnsResolver instance initially owns all QDnsReply objects it returns. It also owns the QThreadPool used to perform the lookups. Initially? What does that mean exactly? When does QDnsResolver stops owning the QDnsReply objects? It means you can use QDnsReply::setParent() to take over ownership of the reply (like QNetworkReply). - I have implemented QDnsReply::abort() to cancel a lookup request Good. I forgot to mention: you can delete the QDnsReply at any time, as both QDnsReply and QDnsResolverRunnable access QDnsReplyPrivate via a QSharedPointer. In that case, wouldn't it make more sense then to return a QDnsReply (so not a pointer)? Hmmm... but that is of course not possible because it derives from QObject, right? Correct, QDnsReply has both an abort() slot and a finished() signal. - Robin mentioned adding a static QDnsResolver::instance() method, does anyone else have an opinion on this? No need if all functions are static. And since they are not? As I argued before: I think they should be, as the class you showed up contains no actual data (from the user's perspective of it, anyway). It causes problems with having to keep the instance alive while the DNS request is running, even though the object itself can not really be interacted with anymore. It doesn't even provide API to know if it can be savely deleted, or if it is still in the background managing running requests. So, when can the API user savely get rid of the requester object? IMHO, it would make sense to have the methods on QDnsResolver be static, and let those static methods reference some private (singleton?) instance of the object that owns stuff like a threadpool and the QDnsReplies. That frees the user from having to care about the lifetime of the resolver object. Currently, QDnsResolver can be deleted at any time without fear of a crash. However if there are outstanding requests, this will block until the QThreadPool has finished. I agree that it looks as though QDnsResolver methods should be static, although it does once again raise the question of QDnsReply ownership since the replies can no longer be owned by the QDnsResolver. However, I am unsure about putting the methods in QDnsReply (even if it gets renamed to just QDns), I don't find it very descriptive in terms of API. Jeremy ___ Development mailing list Development@qt-project.org http://lists.qt-project.org/mailman/listinfo/development
Re: [Development] API review for a new QDnsResolver class
On Wed, Nov 9, 2011 at 11:26 AM, Jeremy Lainé jeremy.la...@m4x.org wrote: Le Nov 9, 2011 à 10:14 AM, André Somers a écrit : On Wed, Nov 9, 2011 at 9:17 AM, Jeremy Lainé jeremy.la...@m4x.org wrote: On 11/08/2011 10:57 PM, Thiago Macieira wrote: On Tuesday, 8 de November de 2011 19:40:13 Jeremy Lainé wrote: - the QNAM-style API seems to be OK Correct, but all functions in QDnsResolver are static. That means they could go into QDnsReply and we could rename the class simply QDns. It worked for Qt 3... The methods are not static, the QDnsResolver instance initially owns all QDnsReply objects it returns. It also owns the QThreadPool used to perform the lookups. Initially? What does that mean exactly? When does QDnsResolver stops owning the QDnsReply objects? It means you can use QDnsReply::setParent() to take over ownership of the reply (like QNetworkReply). OK. I would like it more if that was more explicit in the API (also for QNAM), but ok. I am not a big fan of ::setParent() in such contexts, it doesn't feel natural to me. - I have implemented QDnsReply::abort() to cancel a lookup request Good. I forgot to mention: you can delete the QDnsReply at any time, as both QDnsReply and QDnsResolverRunnable access QDnsReplyPrivate via a QSharedPointer. In that case, wouldn't it make more sense then to return a QDnsReply (so not a pointer)? Hmmm... but that is of course not possible because it derives from QObject, right? Correct, QDnsReply has both an abort() slot and a finished() signal. QFuture solves this by having the QFuture itself be a value class, and have a QFutureWatcher that supplies the signals. Might be overkill though here, and is nicer if the two are combined IMO. - Robin mentioned adding a static QDnsResolver::instance() method, does anyone else have an opinion on this? No need if all functions are static. And since they are not? As I argued before: I think they should be, as the class you showed up contains no actual data (from the user's perspective of it, anyway). It causes problems with having to keep the instance alive while the DNS request is running, even though the object itself can not really be interacted with anymore. It doesn't even provide API to know if it can be savely deleted, or if it is still in the background managing running requests. So, when can the API user savely get rid of the requester object? IMHO, it would make sense to have the methods on QDnsResolver be static, and let those static methods reference some private (singleton?) instance of the object that owns stuff like a threadpool and the QDnsReplies. That frees the user from having to care about the lifetime of the resolver object. Currently, QDnsResolver can be deleted at any time without fear of a crash. However if there are outstanding requests, this will block until the QThreadPool has finished. I agree that it looks as though QDnsResolver methods should be static, although it does once again raise the question of QDnsReply ownership since the replies can no longer be owned by the QDnsResolver. However, I am unsure about putting the methods in QDnsReply (even if it gets renamed to just QDns), I don't find it very descriptive in terms of API. I agree with you on the putting methods in QDnsReply (and calling it QDns). I prefer to keep the reply just that: an object representing a (future) reply to a query, not the query itself. On the ownership: It looks like you will need to have an object with some state somewhere anyway. Somebody has to own that QThreadPool, for instance. So, why not make that object the (first) parent then? Problem then is: who will cleanup the reply objects, and when will that happen, as the user has no control over the lifetime. I don't like having the delete of QDnsResolver block untill are requests are finished. Once again: returning a shared pointer to the QDnsReply object would solve that (the QDnsResolverPrivate class would keep a shared pointer around as long as the request is active), but it seems that is not a favoured solution. Would it be difficult to have QObject::connect support QSharedPointerQObject as the QObject* argument? Or rather, I guess, classes that overload operator-() that return a QObject*? Andre ___ Development mailing list Development@qt-project.org http://lists.qt-project.org/mailman/listinfo/development
Re: [Development] API review for a new QDnsResolver class
On Nov 4, 2011, at 9:37 PM, ext Thiago Macieira wrote: On Friday, 4 de November de 2011 21:01:30 Andre Somers wrote: Me too. My point was, that we have slightly different patters for basically the same sort of thing in different places in Qt. QFuture is currently coupled with QtConcurrent, but is there a strong reason why is must be? I was not privy to that IRC chat, perhaps you could tell us the reasoning why it would not be possible? There's no reason why it has to be coupled with Concurrent. Or, to put in other words, it could be changed to work without Concurrent. However, the problem is, it is currently too tightly coupled with QtConcurrent. Unless someone is volunteering to do this work right now... Actually, is it tightly coupled at all? qfutureinterface.h contains the (undocumented) backend class that run/map/filter uses to produce results for QFuture. That class could be polished up and documented if we want to use QFuture in other places. Morten ___ Development mailing list Development@qt-project.org http://lists.qt-project.org/mailman/listinfo/development
Re: [Development] API review for a new QDnsResolver class
On 11/09/2011 06:43 PM, ext Jeremy Lainé wrote: (...) A/ static QDnsReply* QDnsReply::lookup(QDnsReply::Type, QString); pro: easy to connect to the QDnsReply's signal con: it's entirely up to the user to handle deletion. Judging by your comments above, I doubt you favor it? or B/ static QSharedPointerQDnsReply QDnsReply::lookup(QDnsReply::Type, QString); pro: memory ownership is explicit con: how used are our users to manipulating QSharedPointer with respect to signals and such? I rather favour option B (lessons learned from QNAM), because I think option A might lead to undeleted replies, which is what we experienced in the case of QNetworkReply *QNetworkAccessmanager::get(...). We probably can mitigate the complexity introduced by QSharedPointer by having a simple example in the documentation. However if people more favour option A, that would not be a problem for me either. Peter Jeremy ___ Development mailing list Development@qt-project.org http://lists.qt-project.org/mailman/listinfo/development -- Qt Developer Days 2011 – REGISTER NOW! October 24 – 26, Munich November 29 – December 1, San Francisco Learn more and Register at http://qt.nokia.com/qtdevdays2011 ___ Development mailing list Development@qt-project.org http://lists.qt-project.org/mailman/listinfo/development
Re: [Development] API review for a new QDnsResolver class
On 9 November 2011 18:35, Jeremy Lainé jeremy.la...@m4x.org wrote: C/ you make QDnsReply's constructor public, and let the user manage the lifetime of the object. This is what the Q3Dns API looks like. What I don't like about Q3Dns's API is that it's unclear when the request is actually made. It starts after you return to the event loop, just like all the other socket classes. -- Giuseppe D'Angelo ___ Development mailing list Development@qt-project.org http://lists.qt-project.org/mailman/listinfo/development
Re: [Development] API review for a new QDnsResolver class
Op 9-11-2011 19:35, Jeremy Lainé schreef: On 11/09/2011 07:21 PM, Peter Hartmann wrote: On 11/09/2011 06:43 PM, ext Jeremy Lainé wrote: (...) A/ static QDnsReply* QDnsReply::lookup(QDnsReply::Type, QString); pro: easy to connect to the QDnsReply's signal con: it's entirely up to the user to handle deletion. Judging by your comments above, I doubt you favor it? or B/ static QSharedPointerQDnsReply QDnsReply::lookup(QDnsReply::Type, QString); pro: memory ownership is explicit con: how used are our users to manipulating QSharedPointer with respect to signals and such? I rather favour option B (lessons learned from QNAM), because I think option A might lead to undeleted replies, which is what we experienced in the case of QNetworkReply *QNetworkAccessmanager::get(...). OK, there is always another option: C/ you make QDnsReply's constructor public, and let the user manage the lifetime of the object. This is what the Q3Dns API looks like. What I don't like about Q3Dns's API is that it's unclear when the request is actually made. You mean, like this? //user code QDnsReply* dnsReply = new QDnsReply(this); QDnsResolver::lookup(dnsQuery, dnsReply); Not a big fan... Lets stick to either A or B. *I* prefer B, but that much should be clear by now. I also suggest we ask somebody with more template fu than I can wield to look into if it would be feasable to let this work: QSharedPointerQDnsReply dnsReply = QDnsResolver::lookup(dnsQuery); connect(dnsReply, SIGNAL(finished()), this, SLOT(dnsFinished())); //directly connect, instead of: connect(dnsReply.data(), SIGNAL(finished()), this, SLOT(dnsFinished())); //currently needed connect statement André ___ Development mailing list Development@qt-project.org http://lists.qt-project.org/mailman/listinfo/development
Re: [Development] API review for a new QDnsResolver class
On 11/09/2011 07:41 PM, Giuseppe D'Angelo wrote: On 9 November 2011 18:35, Jeremy Lainéjeremy.la...@m4x.org wrote: C/ you make QDnsReply's constructor public, and let the user manage the lifetime of the object. This is what the Q3Dns API looks like. What I don't like about Q3Dns's API is that it's unclear when the request is actually made. It starts after you return to the event loop, just like all the other socket classes What I mean is that the Q3Dns has setters as well as a constructor with arguments, so for instance: Q3Dns *dns = new Q3Dns; dns-setLabel(foodomain.org);// will a request be triggered if I stop here and don't specify the record type? dns-setRecordType(Q3Dns::); .. later .. dns-setLabel(bardomain.org); // will a new request be issued, since I changed the queried domain? What happens to the existing results? Jeremy ___ Development mailing list Development@qt-project.org http://lists.qt-project.org/mailman/listinfo/development
Re: [Development] API review for a new QDnsResolver class
On 11/04/2011 09:37 PM, Thiago Macieira wrote: On Friday, 4 de November de 2011 21:01:30 Andre Somers wrote: Me too. My point was, that we have slightly different patters for basically the same sort of thing in different places in Qt. QFuture is currently coupled with QtConcurrent, but is there a strong reason why is must be? I was not privy to that IRC chat, perhaps you could tell us the reasoning why it would not be possible? There's no reason why it has to be coupled with Concurrent. Or, to put in other words, it could be changed to work without Concurrent. However, the problem is, it is currently too tightly coupled with QtConcurrent. Unless someone is volunteering to do this work right now... OK so to sum up: - the QNAM-style API seems to be OK - I have implemented QDnsReply::abort() to cancel a lookup request - Robin mentioned adding a static QDnsResolver::instance() method, does anyone else have an opinion on this? - concerning QDnsReply ownership: * using QSharedPointerQDnsReply doesn't really help, it breaks QNAM expectations by making the user needs to hold an explicit reference to the reply * QFuture would be interesting, but it won't be split from QtConcurrent in the foreseable future * as a fallback I propose we stick with the same reply ownership model as QNAM (i.e. the reply is initially owned by the QDnsResolver, but the user can use setParent), it has the advantage of being well-known Unless there are any objections I will push the reworked QDnsResolver to gerrit. Jeremy ___ Development mailing list Development@qt-project.org http://lists.qt-project.org/mailman/listinfo/development
Re: [Development] API review for a new QDnsResolver class
On Tuesday, 8 de November de 2011 19:40:13 Jeremy Lainé wrote: - the QNAM-style API seems to be OK Correct, but all functions in QDnsResolver are static. That means they could go into QDnsReply and we could rename the class simply QDns. It worked for Qt 3... - I have implemented QDnsReply::abort() to cancel a lookup request Good. - Robin mentioned adding a static QDnsResolver::instance() method, does anyone else have an opinion on this? No need if all functions are static. -- 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] API review for a new QDnsResolver class
On Thu, Nov 3, 2011 at 12:40 PM, Jeremy Lainé jeremy.la...@m4x.org wrote:
Based on some initial feedback I received regarding DNS SRV support in Qt,
I have refactored my proposed code and introduced a QDnsResolver class
which I would like to submit for API review. The point of this class is to
provide a QNAM-style asynchronous API to perform DNS lookups.
The resolver object itself looks like:
class Q_NETWORK_EXPORT QDnsResolver : public QObject
{
Q_OBJECT
public:
QDnsResolver(QObject *parent = 0);
~QDnsResolver();
QDnsReply *lookupService(const QString serviceName, const QString
domainName);
private:
Q_DECLARE_PRIVATE(QDnsResolver)
};
The more I think about it, the more I think it is important to fix this:
who is responsible for the lifetime of the QDnsReply object?
This API does not make that clear. I like the pattern in itself (also in
QNAM), but I do think it would be an improvement if we were to use a shared
pointer to the reply object. That at least makes clear who has ownership of
the object, and prevents memory leaks when people don't realize they are
supposed to delete the object.
Alternatively, perhaps a look at QFuture would help. QFuture is another way
results that are not yet ready are handled in Qt, but this time it is
returned as a value instead of as a pointer. It would be nice we could come
up with a single approach for these kinds of things and use that all over
the place...
Another concern is the question why you need a QDnsResolver object at all.
It only has a single method, and it does not seem to benefit from it being
a QObject at all at the moment, unless you plan to mimick the API of QNAM
and add signals and slots on the Resolver object itself. If not, then it
might as well just be a static method of the class, or just a method in a
namespace called QDnsResolver or something like that. Currently, the user
will have to instantiate the QDnsResolver object, and call the
lookupService method. It is not so clear what needs to happen to that
instance after that. Can it be savely deleted again while the request is
still running?
Andre'
___
Development mailing list
Development@qt-project.org
http://lists.qt-project.org/mailman/listinfo/development
Re: [Development] API review for a new QDnsResolver class
On 11/03/2011 08:32 PM, ext Jeremy Lainé wrote: Le Nov 3, 2011 à 7:10 PM, Peter Hartmann a écrit : On 11/03/2011 04:12 PM, ext Thiago Macieira wrote: (...) The DNS query is not the problem. A query is always composed of a domain name being queried, along with the DNS class (Internet, no one uses anything else for real purposes) and the record type. For the record type, it's very easy to just list all types and also say that the user can use other types from the DNS documentation. See the ns_type enum in /usr/include/arpa/nameser.h [1]. Yes, the query is quite simple, that is why I was wondering whether we need a class for it at all. Probably not. In the end I ended just storing the request type and encoded query domain in QDnsReplyPrivate. I have not yet exposed it for lack of a good accessor name. Any suggestions? How about queryType() or requestType() and queryHost() or requestHost()? (I wouldn't go for queryDomain() because that doesn't sound fit for reverse lookups). The RFC and Wikipedia etc. seem to use both 'query' and 'request'... (...) I changed my mind about the replies after all, I find having a single QDnsReply class works well so far (as you probably noticed, I added support for MX and TXT records). Do you see a compelling reason to change this? I am happy with having one QDnsReply class; I think we can get pretty far with some special accessors for SRV and other records, and one generic accessor for simple (QString) and unsupported cases. Then whoever wants to read a currently unsupported response format needs to parse the response string himself, which IMO is not that bad. We can then in the future extend the class with more special accessors if need be. Thanks to Shane's feedback the win32 code now compiles and passes all tests. Jeremy -- Qt Developer Days 2011 – REGISTER NOW! October 24 – 26, Munich November 29 – December 1, San Francisco Learn more and Register at http://qt.nokia.com/qtdevdays2011 ___ Development mailing list Development@qt-project.org http://lists.qt-project.org/mailman/listinfo/development
Re: [Development] API review for a new QDnsResolver class
On Friday, 4 de November de 2011 10:36:29 Peter Hartmann wrote: I am happy with having one QDnsReply class; I think we can get pretty far with some special accessors for SRV and other records, and one generic accessor for simple (QString) and unsupported cases. Then whoever wants to read a currently unsupported response format needs to parse the response string himself, which IMO is not that bad. We can then in the future extend the class with more special accessors if need be. There's no way to provide the unsupported formats in QString. It would need to be QByteArray and, even then, it's useless if it contains a compressed domain name. -- 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] API review for a new QDnsResolver class
On Friday, 4 de November de 2011 10:15:54 André Somers wrote: The more I think about it, the more I think it is important to fix this: who is responsible for the lifetime of the QDnsReply object? This API does not make that clear. I like the pattern in itself (also in QNAM), but I do think it would be an improvement if we were to use a shared pointer to the reply object. That at least makes clear who has ownership of the object, and prevents memory leaks when people don't realize they are supposed to delete the object. Maybe we should pattern this class against something that is already known: http://doc.qt.nokia.com/latest/q3dns.html This class is both the request and the reply and its ownership is clearly known. -- 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] API review for a new QDnsResolver class
Le Nov 4, 2011 à 11:05 AM, Thiago Macieira a écrit : On Friday, 4 de November de 2011 10:36:29 Peter Hartmann wrote: I am happy with having one QDnsReply class; I think we can get pretty far with some special accessors for SRV and other records, and one generic accessor for simple (QString) and unsupported cases. Then whoever wants to read a currently unsupported response format needs to parse the response string himself, which IMO is not that bad. We can then in the future extend the class with more special accessors if need be. There's no way to provide the unsupported formats in QString. It would need to be QByteArray and, even then, it's useless if it contains a compressed domain name. Domain name expansion is handled for known fields, win32 does it itself as far as I know, and for unix I call dn_expand. Jeremy ___ Development mailing list Development@qt-project.org http://lists.qt-project.org/mailman/listinfo/development
Re: [Development] API review for a new QDnsResolver class
On 11/04/2011 11:47 AM, Peter Hartmann wrote: Btw. I think we need the generic accessor anyway because you never know what the format of a TXT response looks like. Q3Dns allowed that only for TXT records, but IMO it would be better to always have an accessor to the raw response data, or at least if the query type is unknown. Access to the raw response data for arbitrary records is not going to work in a cross-platform way. For instance, on Windows the result has already been digested for you and put into a suitable structure depending on the record type: http://msdn.microsoft.com/en-us/library/windows/desktop/ms682082%28v=vs.85%29.aspx Jeremy ___ Development mailing list Development@qt-project.org http://lists.qt-project.org/mailman/listinfo/development
Re: [Development] API review for a new QDnsResolver class
Op 4-11-2011 20:31, Jeremy Lainé schreef:
On 11/04/2011 10:15 AM, André Somers wrote:
The more I think about it, the more I think it is important to fix this: who
is
responsible for the lifetime of the QDnsReply object?
Why not have the same pattern as QNAM for consistency?
How about fixing QNAM to solve this issue there as well then?
This API does not make that clear. I like the pattern in itself (also in
QNAM), but I do
think it would be an improvement if we were to use a shared pointer to the
reply object.
That at least makes clear who has ownership of the object, and prevents
memory leaks
when people don't realize they are supposed to delete the object.
I'm not sure I understand how using a QSharedPointer would clarify the API,
it would lead
to code like this:
void someObject::someMethod()
{
QSharedPointerQDnsReply reply = someResolver-lookupService(X, Y, Z);
connect(reply.data(), SIGNAL(finished()),
this, SLOT(replyFinished()));
}
= surprise, the reply has been deleted!
Has it? Probably not, as the lookup service is still holding on to
another shared pointer to it in order to know where to put the results
of the lookup, right? Using a shared pointer, at least you are sure the
object is valid while either the receiver or the resolver is still
interested in having the object around, and as soon as that is no longer
the case, it will be properly deleted. Using raw pointers, you can not
be sure of either.
dreaming
Would be nice by the way if connect could directly deal with QObjects in
shared pointers...
/dreaming
Alternatively, perhaps a look at QFuture would help. QFuture is another way
results that
are not yet ready are handled in Qt, but this time it is returned as a value
instead of
as a pointer. It would be nice we could come up with a single approach for
these kinds
of things and use that all over the place...
I brought this up with Thiago on IRC, as I quite like the idea of using a
QFuture. However
apparently this is not an option as QtConcurrent is optional.
Me too. My point was, that we have slightly different patters for
basically the same sort of thing in different places in Qt. QFuture is
_currently_ coupled with QtConcurrent, but is there a strong reason why
is must be? I was not privy to that IRC chat, perhaps you could tell us
the reasoning why it would not be possible?
I think the Qt API would benefit from having the same kind of pattern
for waiting for asynchronous results, whether from calculation or from
the network or from some other resource. However, at all times, I think
it must be clear who owns what object, and who is responsible for
cleaning it up.
André
___
Development mailing list
Development@qt-project.org
http://lists.qt-project.org/mailman/listinfo/development
Re: [Development] API review for a new QDnsResolver class
On 11/04/2011 09:01 PM, Andre Somers wrote:
Op 4-11-2011 20:31, Jeremy Lainé schreef:
On 11/04/2011 10:15 AM, André Somers wrote:
The more I think about it, the more I think it is important to fix this:
who is
responsible for the lifetime of the QDnsReply object?
Why not have the same pattern as QNAM for consistency?
How about fixing QNAM to solve this issue there as well then?
This API does not make that clear. I like the pattern in itself (also in
QNAM), but I do
think it would be an improvement if we were to use a shared pointer to the
reply object.
That at least makes clear who has ownership of the object, and prevents
memory leaks
when people don't realize they are supposed to delete the object.
I'm not sure I understand how using a QSharedPointer would clarify the API,
it would lead
to code like this:
void someObject::someMethod()
{
QSharedPointerQDnsReply reply = someResolver-lookupService(X, Y,
Z);
connect(reply.data(), SIGNAL(finished()),
this, SLOT(replyFinished()));
}
= surprise, the reply has been deleted!
Has it? Probably not, as the lookup service is still holding on to
another shared pointer to it in order to know where to put the results
of the lookup, right? Using a shared pointer, at least you are sure the
object is valid while either the receiver or the resolver is still
interested in having the object around, and as soon as that is no longer
the case, it will be properly deleted. Using raw pointers, you can not
be sure of either.
Let's say that another shared pointer is indeed held by the resolver class, at
what point
should we let go of this shared pointer? The answer is probably at some point
after the
finished() signal is emitted, but when exactly?
dreaming
Would be nice by the way if connect could directly deal with QObjects in
shared pointers...
/dreaming
Yep, otherwise we're going to have some confused users!
Jeremy
___
Development mailing list
Development@qt-project.org
http://lists.qt-project.org/mailman/listinfo/development
Re: [Development] API review for a new QDnsResolver class
On Friday, 4 de November de 2011 21:01:30 Andre Somers wrote: Me too. My point was, that we have slightly different patters for basically the same sort of thing in different places in Qt. QFuture is currently coupled with QtConcurrent, but is there a strong reason why is must be? I was not privy to that IRC chat, perhaps you could tell us the reasoning why it would not be possible? There's no reason why it has to be coupled with Concurrent. Or, to put in other words, it could be changed to work without Concurrent. However, the problem is, it is currently too tightly coupled with QtConcurrent. Unless someone is volunteering to do this work right now... -- 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] API review for a new QDnsResolver class
On 11/03/2011 12:40 PM, ext Jeremy Lainé wrote:
Based on some initial feedback I received regarding DNS SRV support in Qt, I
have refactored my proposed code and introduced a QDnsResolver class which
I would like to submit for API review. The point of this class is to provide
a QNAM-style asynchronous API to perform DNS lookups.
The resolver object itself looks like:
class Q_NETWORK_EXPORT QDnsResolver : public QObject
{
Q_OBJECT
public:
QDnsResolver(QObject *parent = 0);
~QDnsResolver();
QDnsReply *lookupService(const QStringserviceName, const
QStringdomainName);
private:
Q_DECLARE_PRIVATE(QDnsResolver)
};
Without having looked at the code in detail, in general I like the idea
of having a low-level DNS query class in Qt.
As Thiago pointed out on IRC, it would complement QHostInfo, as the
latter class uses queries to the system to resolve the name (which might
end up in looking up the name via the hosts file, via DNS, or via other
configuration files).
Currently, there is only lookupService() to perform DNS SRV record types, but
it would be easy to add lookupText() for TXT records, etc.
An open question: should we have multiple lookupXXX() methods, or a single
lookup() which takes a QDnsRequest?
It might make sense to have a generic method for querying any type of
DNS; but then would we need a QDnsRequest class which might be overkill
if we only support SRV and maybe TXT records short term? How complex can
a DNS query possibly get so it would justify creating an own class? I am
not sure about (one generic method plus request class) vs. (several
specific methods) yet...
Peter
(...)
--
Qt Developer Days 2011 – REGISTER NOW!
October 24 – 26, Munich
November 29 – December 1, San Francisco
Learn more and Register at http://qt.nokia.com/qtdevdays2011
___
Development mailing list
Development@qt-project.org
http://lists.qt-project.org/mailman/listinfo/development
Re: [Development] API review for a new QDnsResolver class
On 11/03/2011 04:12 PM, ext Thiago Macieira wrote: (...) The DNS query is not the problem. A query is always composed of a domain name being queried, along with the DNS class (Internet, no one uses anything else for real purposes) and the record type. For the record type, it's very easy to just list all types and also say that the user can use other types from the DNS documentation. See the ns_type enum in /usr/include/arpa/nameser.h [1]. Yes, the query is quite simple, that is why I was wondering whether we need a class for it at all. The problem is the DNS reply. Each query type contains a different kind of data structure that would need to be parsed. It gets very complex very quickly if we want to present the data in a nice, C++ class. I am wondering: According to Jeremy it would be best to have separate classes for different record types; however the way QDnsReply is designed it looks like a generic class for DNS replies with special accessors for specific records like serviceRecords(). @Jeremy: Do you think this is feasible now? I.e. could we have all the accessors combined in one class and have a generic accessor for the QString cases? Or would we have too many methods in the class for the special cases at some point and we should rather have special classes? Peter A 4 bytes containing an IPv4 address (QHostAddress) NSa domain name (QString) CNAME a domain name (QString) SOA two domain names and five integers PTR a domain name (QString) MXan integer and a domain name TXT text (QString) 16 bytes containing an IPv6 address (QHostAddress) SRV two integers and a domain name A6an integer, an IPv6 address and a domain name DNAME a domain name (QString) TKEY, TSIG? And that's just listing the ones that I thought we'd like to support. CNAME and DNAME are likely not to be the query, but they might appear as part of the reply. [1] a that suffers from the creat syndrome. ___ Development mailing list Development@qt-project.org http://lists.qt-project.org/mailman/listinfo/development -- Qt Developer Days 2011 – REGISTER NOW! October 24 – 26, Munich November 29 – December 1, San Francisco Learn more and Register at http://qt.nokia.com/qtdevdays2011 ___ Development mailing list Development@qt-project.org http://lists.qt-project.org/mailman/listinfo/development
