> On March 27, 2016, 10:03 a.m., David Faure wrote:
> > Given how the socket API works, you should only call error() after a call
> > that returns false (e.g. waitForConnected, etc.). As you found out, calling
> > error() at random points in time doesn't give useful information, you get
> > the last error that was ever set, even after 10 successful calls following
> > the call that had an error.
> >
> > Calling setError ourselves is a workaround, which doesn't fit with the Qt
> > socket API (there is no "No error" error code).
> > UnknownSocketError *means* error, it doesn't mean no error.
> >
> > It seems to me (from the description, I didn't read the code attentively)
> > that the right solution is to actually disconnect after a
> > RemoteHostClosedError. Then the state won't be ConnectedState anymore, and
> > isConnected() will work as intended :-)
>
> Albert Astals Cid wrote:
> But it won't, even if you disconnect, the error will still be
> RemoteHostClosedError given that Qt does not reset the error after
> disconnectFromHost (since there's basically no way to mark "no error"), so if
> we reconnect after disconnection we will still have RemoteHostClosedError if
> we ask for the error even if it's not true for this session. Of you mean this
> in connection with the above hack?
>
> Andreas Hartmetz wrote:
> Yeah, what happens here is working around two API mistakes in QTcpSocket:
> that there is no proper "no error" code, and that the error isn't cleared
> when starting a new connection. Apart from settings that may reasonably be
> persistent like e.g. proxy or buffer size settings, a new connection should
> behave exactly the same as deleting and recreating the socket. Otherwise the
> API effectively requires you to delete and recreate a socket to get a clean
> state, which isn't very nice in such a widely used API. (I sometimes do this
> in internal interfaces, it's a nice technique to avoid errors in reset and
> clean up code paths - but it causes a little overhead and more work for
> consumers)
> Since the check here is for a specific error code, UnknownError works as
> a code that isn't that specific one. And if we hack the error code reset, we
> have what we need. I consider that self defense against bad API, not a real
> hack.
>
> David Faure wrote:
> My suggestion is about using the API the way it was intended to be used:
> never calling error() unless a QAbstractSocket method returns false. So
> definitely not calling error() in isConnected(), but only after specific API
> calls that return false.
>
> Albert: if we disconnect, then state() == ConnectedState will be false,
> and therefore isConnected will return false, as intended.
> No need to call error() in that method.
>
> Andreas Hartmetz wrote:
> There does seem to be a way to check whether the socket error is for the
> current connection, sort of... check if the errorString() is empty. That one
> is cleared on close() and AFAICS also set each time a socket error occurs. It
> is also never set on its own - every error is a socket error in
> QAbstractSocket.
> So QAbstractSocket does support lazy error checks, it's just a bit
> strange.
>
> Albert Astals Cid wrote:
> Andreas, that doesn't work, see the code of QIODevice::errorString
>
> if (d->errorString.isEmpty()) {
> #ifdef QT_NO_QOBJECT
> return QLatin1String(QT_TRANSLATE_NOOP(QIODevice, "Unknown
> error"));
> #else
> return tr("Unknown error");
> #endif
> }
>
> Albert Astals Cid wrote:
> > My suggestion is about using the API the way it was intended to be
> used: never calling error() unless a QAbstractSocket method returns false. So
> definitely not calling error() in isConnected(), but only after specific API
> calls that return false.
>
> You can not do that, there's no API call that will return false in this
> case, socket.write returns a valid number of bytes, waitForBytesWritten
> returns true, but if you ask for the error after that, it has been set to
> RemoteHostClosedError, you may argue that is a bug in Qt and that calling
> waitForBytesWritten should properly return false if the calls to
> QAbstractSocketPrivate::canReadNotification,
> QAbstractSocketPrivate::readFromSocket and QNativeSocketEngine::read end up
> setting an error.
>
> > Albert: if we disconnect, then state() == ConnectedState will be false,
> and therefore isConnected will return false, as intended. No need to call
> error() in that method.
>
> Yes, we could do that and it would fix isConnected for the first time,
> the problem is that if after you try to reuse the socket, error() will still
> be set and as there's no way to unset it (or check we have to read it, unless
> as said on the paragraph above, we consider the Qt behaviour a bug and fix it)
>
> David Faure wrote:
> Isn't a signal emitted then, when the socket detects that the remote
> closed the connection?
Neither.
Anyway i found a different way to fix the problems i had with the pop3 slave so
i will close this review request, doesn't make much sense to keep discussing
how to fix the qtcpsocket api problems when it's not strcitly needed for me.
- Albert
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/127501/#review94042
-----------------------------------------------------------
On March 26, 2016, 5:29 p.m., Albert Astals Cid wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/127501/
> -----------------------------------------------------------
>
> (Updated March 26, 2016, 5:29 p.m.)
>
>
> Review request for KDE Frameworks.
>
>
> Repository: kio
>
>
> Description
> -------
>
> Qt sockets returns ConnectedState even when error is set to
> RemoteHostClosedError after a write operation.
>
> So make ::isConnected also check for RemoteHostClosedError.
>
> This has the consequence that we have to create/delete the socket since Qt
> sockets have no way to reset their error state so if we want to reuse the
> slave to connect again it will never work with the same socket since it will
> always say RemoteHostClosedError.
>
> I need this for https://git.reviewboard.kde.org/r/127502/
>
>
> Diffs
> -----
>
> src/core/tcpslavebase.cpp b9be69d
>
> Diff: https://git.reviewboard.kde.org/r/127501/diff/
>
>
> Testing
> -------
>
> Seesms to work fine and the pop3 bugfix i have also works fine.
>
>
> Thanks,
>
> Albert Astals Cid
>
>
_______________________________________________
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
