Fix QLocalSocket problem under Windows #2197

llloret · 2016-06-21T13:55:39Z

Still Work in Progress.

For now, it fixes the Windows crash, so this should enable us to move
the Windows development to master again, which we should try to do ASAP.

Since QLocalSocket under Windows is unreliable (it might cut messages),
it is not advised to use QDataStream.

I have changed it to a messaging system based on strings and REs. At the
beginning of each message I put the length of the message, so that the
receiving side knows how much to retrieve.

The change for now is only for the messages coming from SCProcess to
sc_ipc_client direction. I don't know if something similar needs to be
done in the other direction, or where..., please advise

For now, it fixes the Windows crash, so this should enable us to move the Windows development to master again, which we should try to do ASAP. Since QLocalSocket under Windows is unreliable (it might cut messages), it is not advised to use QDataStream. I have changed it to a messaging system based on strings and REs. At the beginning of each message we put the length of the message, so that the receiving side knows how much to retrieve. The change for now is only for the messages coming from SCProcess to sc_ipc_client direction. *I don't know if something similar needs to be done in the other direction, or where...*

llloret · 2016-06-21T13:58:27Z

@timblechmann, I could really use your input on this...

I am not very familiar with what the QLocalSockets are used for, so I am not sure if this is enough... I think I have fixed the case for the updateText and updateSel selectors, that was crashing Windows, but I am not sure if I need to make the change somewhere else.

I have tried to make the change as compact as possible, following your advice on the previous stab at fixing this. This is still using QLocalSocket, with some changes to make it usable under Windows.

What do you think? Thanks in advance,
Luis

timblechmann · 2016-06-21T14:01:35Z

editors/sc-ide/primitives/sc_ipc_client.cpp

-        QDataStream in ( &receivedData );
-        in.setVersion ( QDataStream::Qt_4_6 );
-        QString selector;
+    static int readSize = 0;


move the static variable to the class. static variables aren't free

timblechmann · 2016-06-21T14:14:56Z

my gut feeling tells me that regular expressions add quite some overhead and in a way i'd prefer to correctly this in qt ... and if this means enabling the wince workaround for localsockets for win32. afaict this was never raised to the qt developers and nobody actually checked if that workaround would be feasible. with your approach, every communication over the local sockets will have to be guarded in that way. one may also run into those issue when evaluating huge files ...

personally i'd recommend to compile qt from sources and possibly provide qt binaries of recent qt version with qtwebkit enabled. but @llloret, as mentioned before i don't care about the community anymore, after people constantly complained to me about not caring about the community. so it's your call.

llloret · 2016-06-21T14:30:21Z

Thanks for your input, @timblechmann, I really appreciate it.

In case we wanted to pursue the approach I am taking... where else do you think I should makes this change? I am not sure what the LocalSockets are used for, really.

I looked into doing the recompilation stuff, and I saw I had to change some Qt project files (socket.pri) to trigger the workaround on Win32, and that left me a bit uneasy... and didn't pursue further.

timblechmann · 2016-06-21T14:41:57Z

@llloret, local sockets are used for the communication between IDE and sclang. you might have to go through the sources to find the corresponding places (it has been a few years since jakob and me wrote that code, i'm not sure about all details).

recompiling qt: it boils down to add one preprocessor definition. they seem to have a workaround in place, but only enable it for wince, but not for win32 ... qt takes a bit to compile, but in general the process is quite straight-forward once you have all the dependencies installed (https://doc.qt.io/qt-5/windows-building.html).
but then: it should really be reported upstream ...

llloret · 2016-06-21T14:59:02Z

@timblechmann, I think that the recompilation needs more than a preprocessor definition: from what I can see different files are put in SRC in socket.pri (apart from defining QT_LOCALSOCKET_TCP), depending on the system (win32, winrt, unix, ...). So I think it needs tinkering with the socket.pri file, to make win32 use the appropriate qlocalserver_tcp files. The define alone would not do it, from what I saw.

Regarding reporting upstream... the thing is that I am not sure if this is a genuine bug, or just that it does not guarantee message oriented delivery, like TCP. That's why adding the length of the message at the beginning looks like a good solution (quite common on non message oriented protocols).

But instead of doing the QDataStreamover the socket directly do it over a ByteArray, and prepend the length of the ByteArray, so that the receiver knows how much data to read

llloret · 2016-06-21T22:32:56Z

Ok, so I realized that I don't need the Regular Expression stuff. I can just use QDataStream over a QByteArray, instead of directly over the socket, and that way I know how much data I am writing, so I can put the size in front of the data. That way the receiving side knows how much to fetch and feed into a QDataStream to read it back.

This makes the change even more compact, and more similar to the original.

Create aux files to put some helper functions to help with LocalSockets. Add the new approach to the SingleGuardInstance case.

llloret · 2016-06-22T14:13:26Z

Hi, @timblechmann, what do you think of the current state of the changes now? I removed the Regular Expression stuff and went back to hte original approach, but instead of having the QDataStreams over the socket directly, I stream over a QByteArray first, to see the size and then send over the socket prepending the size.

This way, the approach keeps the same spirit as your original implementation, just adding the size in front to deal with unreliable message boundaries.

I have put some new auxiliary functions for this in a separate file.

I think there is one more case to deal with, which is the introspection messages. I have already changed the SingleInstanceGuard, which required a very minor change. Will try to work on that later this week.

But getting runtime warnings on setTextMirrorForDocument, so probably broke something

timblechmann · 2016-06-22T16:36:15Z

editors/sc-ide/primitives/sc_ipc_client.hpp

@@ -54,6 +54,7 @@ private Q_SLOTS:
    void updateDocText( const QVariantList & argList );
    void updateDocSelection( const QVariantList & argList );

+    int mReadSize;


int mReadSize = 0; ... in-class initialiser are a bit more robust ... we have 2016, no need to use pre-c++11 constructs

timblechmann · 2016-06-22T16:39:27Z

quickly went through it ... looks much better now!

llloret · 2016-06-22T18:50:19Z

Yep, once I realized that I could QDataStream in and out of QByteArrays everything made more sense...

llloret · 2016-06-22T19:02:07Z

Ok, so I think that this is ready for more serious testing beyond me. So far I have tested it only on WIndows, and it looks good. I can try to do a Linux build, but not sure when.

I can't test on Mac at all, so if someone could try that, it would be great.

As I have already said, to me it is very important to get this on master ASAP to have Windows back into said master, and avoid desyncing again.

So it would be great if we get some LGTMs or more feedback on this...

crucialfelix · 2016-06-27T15:05:44Z

Will test OS X as soon as I find a moment. Thanks luis and tim !

llloret · 2016-06-27T15:38:03Z

Brilliant, let me know how it goes. You will be the first one testing it on Mac...(I don´t have one), so be patient...!.

crucialfelix · 2016-08-20T10:22:10Z

Runs fine on OS X. I've just started the IDE, executed code, booted server, played some sounds.

Anything odd I should test against ? There are no more regexp.

ready to merge, ja ?

llloret · 2016-08-20T11:02:08Z

Hi,

I removed the regex approach, since I ended up using a more elegant and
simple way.

Great that it works on osx.

Yes, I would merge this as soon as we can. At the moment I think it is one
of the biggest issues on portability between platforms.
Luis

On Sat, 20 Aug 2016, 11:22 Chris Sattinger, notifications@github.com
wrote:

Runs fine on OS X. I've just started the IDE, executed code, booted
server, played some sounds.

Anything odd I should test against ? There are no more regexp.

ready to merge, ja ?

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#2197 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/ABLl8_nORAB9YlqblTxhwMpkVIwjiuTlks5qhtVXgaJpZM4I6wv-
.

tirpen · 2016-08-20T17:04:18Z

Hi,

It seems that this last commit breaks Linux builds at least for x86_64:

In file included from /home/aschneider/Downloads/supercollider/common/localsocket_utils.cpp:1:0: /home/aschneider/Downloads/supercollider/common/localsocket_utils.hpp:4:22: fatal error: QByteArray: No such file or directory #include <QByteArray> ^ compilation terminated. make[2]: *** [lang/CMakeFiles/libsclang.dir/__/common/localsocket_utils.cpp.o] Error 1 make[1]: *** [lang/CMakeFiles/libsclang.dir/all] Error 2 make: *** [all] Error 2

vivid-synth · 2016-08-20T18:18:19Z

common/localsocket_utils.cpp

+}
+
+QByteArray IntToArray(qint32 source)
+{


Won't using Qt primitives in "common" break builds that have -DSC_QT=False?

vivid-synth · 2016-08-20T18:43:16Z

Yes, this breaks Linux and/or non-Qt builds: I see the same error as @aschneiderg

nhthn · 2016-08-20T18:54:19Z

Working here on Linux x86_64, SC_QT is on.

tirpen · 2016-08-20T18:57:34Z

@vivid-synth yes, it breaks non-Qt builds. Not being able to test Qt here as my repos use 5.6 which is known to not build. My params:

cmake -L -DCMAKE_BUILD_TYPE="Release" -DBUILD_TESTING=OFF -DSSE=OFF -DSSE2=OFF -DSUPERNOVA=ON -DNATIVE=ON -DSC_WII=OFF -DSC_IDE=OFF -DSC_QT=OFF -DSC_ED=OFF -DSC_EL=OFF -DSC_VIM=ON

vivid-synth · 2016-08-20T19:05:27Z

lang/CMakeLists.txt

@@ -83,6 +83,7 @@ set(sclang_sources
 	${CMAKE_SOURCE_DIR}/common/SC_StringBuffer.cpp
 	${CMAKE_SOURCE_DIR}/common/SC_StringParser.cpp
 	${CMAKE_SOURCE_DIR}/common/SC_TextUtils.cpp
+	${CMAKE_SOURCE_DIR}/common/localsocket_utils.cpp


Do we actually need this line? 'lang' seems not to have any changes: all the changes are in 'sc-ide'.

vivid-synth · 2016-08-20T19:59:53Z

FWIW removing the above line from lang/CMakeLists.txt fixes the build on non-Qt installs, but I'd still also recommend we not keep anything Qt in "common".

crucialfelix · 2016-08-20T20:12:26Z

Ah nuts, sorry. I hope we can figure out a subsequent PR to restore that. I'm interested in a small nonqt build myself.

vivid-synth · 2016-08-20T20:28:52Z

I made changes to fix this but can't test: maybe someone with Qt can try?: #2299

llloret · 2016-08-22T08:59:17Z

Hi, is there something I should do here? I don't know what this non-qt build is...

crucialfelix · 2016-08-22T09:05:56Z

No, don't worry about it. @vivid-synth has nearly got it solved.

non-qt build means building sclang without Qt - it is much smaller.

we should have it in the travis test. maybe as the first one to get tested since it will build faster and fail faster.

vivid-synth · 2016-08-22T13:32:10Z

@crucialfelix very good idea; I've created a ticket: #2306

timblechmann reviewed Jun 21, 2016
View reviewed changes

apply feedback from Tim

49135b0

crucialfelix added the Work In Progress (WIP) - don't merge yet label Jun 21, 2016

llloret added 3 commits June 21, 2016 16:02

fix compile in Linux and remove static variable

816fe75

go back to using QDataStream instead of RE

8da8ad9

But instead of doing the QDataStreamover the socket directly do it over a ByteArray, and prepend the length of the ByteArray, so that the receiver knows how much data to read

Remove RE for good and whitespace removal

cb8a5fc

refactor localsocket code and add to open files case

a0aaf05

Create aux files to put some helper functions to help with LocalSockets. Add the new approach to the SingleGuardInstance case.

llloret added 6 commits June 22, 2016 16:03

renamed template function for consistency

9ef9d3d

implemented other side of channel

5a3d5cb

But getting runtime warnings on setTextMirrorForDocument, so probably broke something

small fix

4dea208

add missing onResponse call

a7dcfa3

fix whitespace

fd78eb5

remove DEBUG_IPC definition

282cabe

timblechmann reviewed Jun 22, 2016
View reviewed changes

updated with Tim's feedback

7a0cfb5

crucialfelix added waiting for testing os: Windows and removed Work In Progress (WIP) - don't merge yet labels Jun 27, 2016

crucialfelix added this to the 3.8 milestone Jun 27, 2016

crucialfelix removed the waiting for testing label Aug 20, 2016

crucialfelix merged commit d041641 into supercollider:master Aug 20, 2016

vivid-synth reviewed Aug 20, 2016
View reviewed changes

vivid-synth mentioned this pull request Aug 20, 2016

Move Qt primitives out of "common" to fix non-Qt builds #2299

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix QLocalSocket problem under Windows #2197

Fix QLocalSocket problem under Windows #2197

llloret commented Jun 21, 2016

llloret commented Jun 21, 2016

timblechmann Jun 21, 2016

timblechmann commented Jun 21, 2016

llloret commented Jun 21, 2016

timblechmann commented Jun 21, 2016

llloret commented Jun 21, 2016

llloret commented Jun 21, 2016 •

edited

Loading

llloret commented Jun 22, 2016

timblechmann Jun 22, 2016

timblechmann commented Jun 22, 2016

llloret commented Jun 22, 2016

llloret commented Jun 22, 2016

crucialfelix commented Jun 27, 2016

llloret commented Jun 27, 2016 via email

crucialfelix commented Aug 20, 2016

llloret commented Aug 20, 2016

tirpen commented Aug 20, 2016

vivid-synth Aug 20, 2016

vivid-synth commented Aug 20, 2016

nhthn commented Aug 20, 2016

tirpen commented Aug 20, 2016

vivid-synth Aug 20, 2016

vivid-synth commented Aug 20, 2016 •

edited

Loading

crucialfelix commented Aug 20, 2016

vivid-synth commented Aug 20, 2016

llloret commented Aug 22, 2016

crucialfelix commented Aug 22, 2016

vivid-synth commented Aug 22, 2016

Fix QLocalSocket problem under Windows #2197

Fix QLocalSocket problem under Windows #2197

Conversation

llloret commented Jun 21, 2016

llloret commented Jun 21, 2016

timblechmann Jun 21, 2016

Choose a reason for hiding this comment

timblechmann commented Jun 21, 2016

llloret commented Jun 21, 2016

timblechmann commented Jun 21, 2016

llloret commented Jun 21, 2016

llloret commented Jun 21, 2016 • edited Loading

llloret commented Jun 22, 2016

timblechmann Jun 22, 2016

Choose a reason for hiding this comment

timblechmann commented Jun 22, 2016

llloret commented Jun 22, 2016

llloret commented Jun 22, 2016

crucialfelix commented Jun 27, 2016

llloret commented Jun 27, 2016 via email

crucialfelix commented Aug 20, 2016

llloret commented Aug 20, 2016

tirpen commented Aug 20, 2016

vivid-synth Aug 20, 2016

Choose a reason for hiding this comment

vivid-synth commented Aug 20, 2016

nhthn commented Aug 20, 2016

tirpen commented Aug 20, 2016

vivid-synth Aug 20, 2016

Choose a reason for hiding this comment

vivid-synth commented Aug 20, 2016 • edited Loading

crucialfelix commented Aug 20, 2016

vivid-synth commented Aug 20, 2016

llloret commented Aug 22, 2016

crucialfelix commented Aug 22, 2016

vivid-synth commented Aug 22, 2016

llloret commented Jun 21, 2016 •

edited

Loading

vivid-synth commented Aug 20, 2016 •

edited

Loading