#653 Rewrite Qt socket linking and waiting code

[Dan Sebald]

The attached patch is meant to clean up a logt of the Qt code that launches and establishes a link with gnuplot_qt server as well as the mechanisms for waiting on requested data to come back from gnuplot_qt. There is only a small change to the application code which is to send the name of the server it constructs to standard out so that qt_term can read the name.

Here is a summary of mods:

• I changed the QProcess to be a conventional one rather than a disconnected one. That way there is still some access to the process so that the constructed server name can be retrieved from the process itself rather than having to duplicate the construction of the name in multiple places. We can then use the Qt waiting mechanisms and get rid of the custom while loop that waits for an external server to appear.

• Note: In qt_flushOutBuffer() the flush() is necessary because there is no event loop. If there were an event loop, just socket.write() would be enough.

• I commented out

        // Avoid dead-locking when no more data is available
        if (qt->socket.bytesToWrite() > 0)
            qt->socket.waitForBytesWritten(-1);

because I don't think it is necessary. I left it there just in case it is needed on some systems. I doubt it, though.

• I added

        qt->socket.disconnectFromServer();

to the end of qt_atexit(). That routine waits for all pending data to be sent before returning and complains if one attempts to wait on disconnect. So, an additional wait-on-disconnect doesn't seem necessary.

• I altered the way that qt_term waits on gnuplot_qt when it wants font property info. (We are going to need to add a similar routine in order to get the true terminal size back from gnuplot_qt term in to fix the issue Ethan noted.) I make use of the proper timeout mechanism for Qt rather than introducing "micro sleep" and timeouts of 0 or -1. If one reviews that hunk of code they'll realize it is pretty conventional, i.e., keep reading bytes until enough are read to fill sizeof(gp_event_t). It turns out that one read typically get all of the bytes necessary.

• Even after this change, I don't like the fact that qt_sendFont() discards events. It is usually carriage returns from holding the Enter key down and an event slips in before gnuplot_qt receives GEFontMetricRequest. However, I'm not sure what to do without some kind of queueing mechanism.

• Similar to the waiting on font info, I changed this while loop:

        while (qt->socket.bytesAvailable() >= (int)sizeof(gp_event_t)) {

because, as with a commenter on the gnuplot developers list, that construct doesn't sit well with me. Beyond seeming like it could get stuck in certain situations (maybe not), I don't like that it is whirling away waiting for the number of bytes to be sufficient. We should be using the Qt mechanism to do any type of waiting rather than doing polling.

• I changed that construct for Windows as well. It looks like almost an exact copy so I suspect it shouldn't break Windows Qt term. But if it breaks something, we'll have to do another rev.

• The only thing I worry about (and I've left a note in the code) is that when waiting for responses from gnuplot_qt (such as font props) if there was a preceeding plot that has a huge amount of data. If the plot doesn't complete in the timeout, then there could be an error. The proper thing would be to dynamically adjust the wait time. But let's hold off on that scheme. I defined DEFAULT_TIMEOUT at the top of qt_term.cpp and we can lengthen that if need be.

• There may be a chance that Windows persist works correctly now. I changed the qt_atexit() function slightly to use Qt routines

        // This Qt function waits until pending data to write is complete.
        qt->socket.disconnectFromServer();

Before that change it seemed like qt_atexit() might be wrapping things up too briskly, not giving enough time for proper interchange and disconnect. Perhaps Windows automatically deletes the gnuplot_qt process without proper disconnect, or more likely the persist command wasn't making its way to gnuplot_qt.

[Ethan Merritt]

Comments on the patch:

In qt_connectToServer(const QString& server, bool retry = true)
your patch removes the delay loop that allowed it to finally work on OSX. Possibly you changed something elsewhere that will make the timeout in socket.waitForConnected() actually work now, but hitherto it has returned immediately regardless of the timeout value. We'll see what Mojca reports.

Same thing in the wait-for-font-info code. Your comment says "the one thing that could cause a significant delay is ...", but this is not true. The thing causing the delay on OSX is the slow response of the system font daemon to the very first query for font metrics. There is no previous plot at this point; we're still initializing the first one. I think it's extremely unlikely that the iteraction time of the display loop is ever limiting. Even for a complex plot that's probably only a matter of msecs. The font server is holding things up for 10s of seconds.

In qt_graphics()
you changed the order of events "send Deactivate to previous server; connect to new server" so that now you're sending the Deactivate to the new server instead. I don't see how that can be correct.

I don't understand the code changes at lines 1116-1155. So far as I know the previous code in this region was working fine. Do have some reason to believe that the communication was getting jammed by incomplete transfer of an event packet? Before if this happened (which I don't think it ever did) the code would just defer all action until the rest of the packet arrived. Now you have it looping locally, which seems like a regression if it ever actually triggers.

But my comments are all on the basis of just looking at the code. The proof of the pudding is in whether it runs better on OSX.

Trivial:
qtterminal/qt_term.cpp: In function ‘void execGnuplotQt()’:
qtterminal/qt_term.cpp:220:9: warning: unused variable ‘pid’ [-Wunused-variable]
qint64 pid;
^

[Dan Sebald]

Well, Mojca said OSX did have problems...

It seemed to get past the qt_connectToServer problem. The issue in the code is

if (!qt->socket.waitForConnected(-1))

returns immediately because the external server gnuplot_qt wasn't yet created and registered. However, with the changeset, the code won't get to this test until it is certain that the server is available. Hence, I replaced the infinite wait with a timed-out wait.

If the font server is beyond 10 seconds, then I didn't choose DEFAULT_TIMEOUT long enough. On the other hand, the code must not be doing something right with OSX fonts if it is taking 10 seconds. (I've no knowledge about OSX fonts.)

"In qt_graphics() you changed the order of events "send Deactivate to previous server; connect to new server""

OK, I didn't understand that. That could probably use a short comment or two in the code. The ensureOptionsCreated(); comes first but I couldn't figure out why qt_connectToServer() shouldn't follow that.

"Do have some reason to believe that the communication was getting jammed by incomplete transfer of an event packet?"

I did get the message once or twice but that could be attributed to some not-quite-correct-at-the-time code.

"Before if this happened (which I don't think it ever did)"

I'm not sure it ever happened either.

"the code would just defer all action until the rest of the packet arrived."

I'm not quite sure it did that. It does if it reaches the second inner loop, but before entering the bigger loop is:

- if (qt->socket.bytesAvailable() < (int)sizeof(gp_event_t)) {
-   qDebug() << "Error: short read from gnuplot_qt socket";
-   return '\0';
- }

which effectively discards the data read up to that point by exiting the code via return.

"Now you have it looping locally, which seems like a regression if it ever actually triggers."

The local loop has a wait in the test condition:

if (!b_read && !qt->socket.waitForReadyRead(DEFAULT_TIMEOUT))

rather than circle around right away.

In any case, it's easy to pull out the few hunks for establishing the socket link. Let me know if I should create a separate patch for that and yank the lines from the attached patch.