On 13-10-03 05:31 AM, Nick Treleaven wrote:
Moving to devel list.
On 02/10/2013 22:58, Matthew Brush wrote:
On 13-10-02 04:40 AM, Nick Treleaven wrote:
On 01/10/2013 22:28, Matthew Brush wrote:
On 13-10-01 05:34 AM, Nick Treleaven wrote:
Just checked my email, was away last week. I'll have a look at it, but I doubt the GLib spawning has been fixed. Either way it should be definitely be tested before reverting to the broken GLib spawn. I know GLib spawn doesn't work with my version of GLib/gtk, which isn't that old (1yr?).
Locally on my Win7 VM I have reverted the patch and removed all the existing SYNC_SPAWN-guarded stuff and rid of win32-specific and to just use GLib spawning same code on all platforms and it runs easily as good as it does before the changes (just did some basic testing though). It's still not nice and non-blocking like it is on Linux, but seems to be no worse-off after removing like 200 lines of code :)
If you push your branch I'll try it. I'll need to use it for a few weeks to be convinced though. I remember the problem only occurred for certain things, maybe running make or non-gnu binaries or something.
For testing purposes it should be as simple as disabling the SYNC_SPAWN workaround[1].
I tried that a while ago, it didn't work. That was when I added the SYNC_SPAWN macro.
"Didn't work" how? The only time it didn't work for me was with old GTK+ 2.16 binaries the gspawn-win32-helper program would crash instantly causing a Microsoft dialog to come up asking to debug the executable. It would be interesting to investigate the original original problem and maybe we can find a solution that gets rid of the other work-arounds all together.
In the meantime, I suggest reverting the "fix" until the actual problem is identified and a fix is properly implemented. There's only 1 extremely minor conflict when reverting.
IMO changing/breaking path quoting is far less important than having a
Maybe to you, but it's very common on Windows to have spaces in the path names, it's even the default on Windows XP (Documents and Settings). To most people (ex. the bug reporters) it seems outright broken now, whereas before it worked.
spawn that doesn't block Geany or outright hang. This definitely
It would be nice to figure out *why* it was hanging and fix *that* problem though, not just drop the existing workaround code and write 3rd workaround that's arguably worse[1] than the original issue.
happened very frequently when using dmd from dlang.org. See:
https://github.com/geany/geany/commit/a3664fae9ece396952d732cc937e63192d8c6f...
I don't remember seeing any bug reports for this, so it's possible it's limited to you or the specific application you were using, maybe a bug in a certain version of DMD? The commit messages doesn't explain what the actual root of the problem was or why it could not be fixed properly, or what was tried to fix it before abandoning the existing code.
BTW the blocking and hanging was with the custom windows process spawning code, not with GLib's spawning. GLib IME just doesn't report the output properly, or at least not both stdout and stderr for all binaries.
What did you try to fix it before dropping asynchronous spawning? How did it improperly report its output? Did it cause the handlers for the IO channels not to always fire or something? What programs didn't it work with? What versions were tested? Can it be reproduced in a clean sample program outside of Geany?
BTW, do you have a link to the upstream bug report for this issue? I googled a lot, and besides this old fixed issue[2], I don't seem to find much besides a few Geany-related things that just say "don't work" without additional details. Did anyone from Geany report the bug upstream or make note of which upstream bug is causing the original original issue?
Cheers, Matthew Brush
[1]: Causes a command prompt to popup, doesn't support filenames with spaces, uses a temp file, uses system(), leaves a bunch of complicated dead code, now 3rd way to spawn process, etc. [2]: https://bugzilla.gnome.org/show_bug.cgi?id=510664
cc'ing the devel list.
Hi Matt, I'll have to reply to this email properly tomorrow.
In the meantime, there are 3 spawning variants: I'm confident that the custom process spawning code is definitely seriously buggy. I really do need the system() code. I tried, but couldn't fix it. I know that last time I tried it, the g_spawn didn't capture enough output. At least one user - the one that contributed it - also had problems with g_spawn capturing, and wrote the custom process spawning.
I suggest picking whichever one you and others think is best as the default, but have a hidden pref to enable at least the system() version. If you don't want to do that, I'll try to find some time, but I'm not sure when yet.
On 03/10/2013 15:43, Matthew Brush wrote:
On 13-10-03 05:31 AM, Nick Treleaven wrote:
Moving to devel list.
On 02/10/2013 22:58, Matthew Brush wrote:
On 13-10-02 04:40 AM, Nick Treleaven wrote:
On 01/10/2013 22:28, Matthew Brush wrote:
On 13-10-01 05:34 AM, Nick Treleaven wrote:
Just checked my email, was away last week. I'll have a look at it, but I doubt the GLib spawning has been fixed. Either way it should be definitely be tested before reverting to the broken GLib spawn. I know GLib spawn doesn't work with my version of GLib/gtk, which isn't that old (1yr?).
Locally on my Win7 VM I have reverted the patch and removed all the existing SYNC_SPAWN-guarded stuff and rid of win32-specific and to just use GLib spawning same code on all platforms and it runs easily as good as it does before the changes (just did some basic testing though). It's still not nice and non-blocking like it is on Linux, but seems to be no worse-off after removing like 200 lines of code :)
If you push your branch I'll try it. I'll need to use it for a few weeks to be convinced though. I remember the problem only occurred for certain things, maybe running make or non-gnu binaries or something.
For testing purposes it should be as simple as disabling the SYNC_SPAWN workaround[1].
I tried that a while ago, it didn't work. That was when I added the SYNC_SPAWN macro.
"Didn't work" how? The only time it didn't work for me was with old GTK+ 2.16 binaries the gspawn-win32-helper program would crash instantly causing a Microsoft dialog to come up asking to debug the executable. It would be interesting to investigate the original original problem and maybe we can find a solution that gets rid of the other work-arounds all together.
In the meantime, I suggest reverting the "fix" until the actual problem is identified and a fix is properly implemented. There's only 1 extremely minor conflict when reverting.
IMO changing/breaking path quoting is far less important than having a
Maybe to you, but it's very common on Windows to have spaces in the path names, it's even the default on Windows XP (Documents and Settings). To most people (ex. the bug reporters) it seems outright broken now, whereas before it worked.
spawn that doesn't block Geany or outright hang. This definitely
It would be nice to figure out *why* it was hanging and fix *that* problem though, not just drop the existing workaround code and write 3rd workaround that's arguably worse[1] than the original issue.
happened very frequently when using dmd from dlang.org. See:
https://github.com/geany/geany/commit/a3664fae9ece396952d732cc937e63192d8c6f...
I don't remember seeing any bug reports for this, so it's possible it's limited to you or the specific application you were using, maybe a bug in a certain version of DMD? The commit messages doesn't explain what the actual root of the problem was or why it could not be fixed properly, or what was tried to fix it before abandoning the existing code.
BTW the blocking and hanging was with the custom windows process spawning code, not with GLib's spawning. GLib IME just doesn't report the output properly, or at least not both stdout and stderr for all binaries.
What did you try to fix it before dropping asynchronous spawning? How did it improperly report its output? Did it cause the handlers for the IO channels not to always fire or something? What programs didn't it work with? What versions were tested? Can it be reproduced in a clean sample program outside of Geany?
BTW, do you have a link to the upstream bug report for this issue? I googled a lot, and besides this old fixed issue[2], I don't seem to find much besides a few Geany-related things that just say "don't work" without additional details. Did anyone from Geany report the bug upstream or make note of which upstream bug is causing the original original issue?
Cheers, Matthew Brush
[1]: Causes a command prompt to popup, doesn't support filenames with spaces, uses a temp file, uses system(), leaves a bunch of complicated dead code, now 3rd way to spawn process, etc. [2]: https://bugzilla.gnome.org/show_bug.cgi?id=510664
On 4 October 2013 03:00, Nick Treleaven nick.treleaven@btinternet.comwrote:
cc'ing the devel list.
Hi Matt, I'll have to reply to this email properly tomorrow.
In the meantime, there are 3 spawning variants: I'm confident that the custom process spawning code is definitely seriously buggy. I really do need the system() code. I tried, but couldn't fix it. I know that last time I tried it, the g_spawn didn't capture enough output. At least one user - the one that contributed it - also had problems with g_spawn capturing, and wrote the custom process spawning.
I suggest picking whichever one you and others think is best as the default, but have a hidden pref to enable at least the system() version. If you don't want to do that, I'll try to find some time, but I'm not sure when yet.
Hi Nick,
The option sounds like a good idea as an interim solution. Whoever gets the time first does it.
Cheers Lex
[...]
On 03/10/2013 18:00, Nick Treleaven wrote:
In the meantime, there are 3 spawning variants: I'm confident that the custom process spawning code is definitely seriously buggy. I really do need the system() code. I tried, but couldn't fix it. I know that last time I tried it, the g_spawn didn't capture enough output. At least one user - the one that contributed it - also had problems with g_spawn capturing, and wrote the custom process spawning.
I suggest picking whichever one you and others think is best as the default, but have a hidden pref to enable at least the system() version. If you don't want to do that, I'll try to find some time, but I'm not sure when yet.
https://github.com/geany/geany/pull/174
Also, sorry I didn't realize the quoting bugs were due to my system() change. Thanks Matt for sending me a private mail about it, and convincing me it needed resolving.
Now I'll answer the rest of your mail Matt.
On 03/10/2013 15:43, Matthew Brush wrote:
On 13-10-03 05:31 AM, Nick Treleaven wrote:
On 02/10/2013 22:58, Matthew Brush wrote:
On 13-10-02 04:40 AM, Nick Treleaven wrote:
On 01/10/2013 22:28, Matthew Brush wrote:
On 13-10-01 05:34 AM, Nick Treleaven wrote:
Just checked my email, was away last week. I'll have a look at it, but I doubt the GLib spawning has been fixed. Either way it should be definitely be tested before reverting to the broken GLib spawn. I know GLib spawn doesn't work with my version of GLib/gtk, which isn't that old (1yr?).
Locally on my Win7 VM I have reverted the patch and removed all the existing SYNC_SPAWN-guarded stuff and rid of win32-specific and to just use GLib spawning same code on all platforms and it runs easily as good as it does before the changes (just did some basic testing though). It's still not nice and non-blocking like it is on Linux, but seems to be no worse-off after removing like 200 lines of code :)
If you push your branch I'll try it. I'll need to use it for a few weeks to be convinced though. I remember the problem only occurred for certain things, maybe running make or non-gnu binaries or something.
For testing purposes it should be as simple as disabling the SYNC_SPAWN workaround[1].
I tried that a while ago, it didn't work. That was when I added the SYNC_SPAWN macro.
"Didn't work" how? The only time it didn't work for me was with old GTK+ 2.16 binaries the gspawn-win32-helper program would crash instantly causing a Microsoft dialog to come up asking to debug the executable. It would be interesting to investigate the original original problem and maybe we can find a solution that gets rid of the other work-arounds all together.
Didn't capture the output properly - stdout or stderr were missing.
In the meantime, I suggest reverting the "fix" until the actual problem is identified and a fix is properly implemented. There's only 1 extremely minor conflict when reverting.
IMO changing/breaking path quoting is far less important than having a
Maybe to you, but it's very common on Windows to have spaces in the path names, it's even the default on Windows XP (Documents and Settings). To most people (ex. the bug reporters) it seems outright broken now, whereas before it worked.
See my other mail, I made a pull request. I tend to put my code in C:\git, but obviously others use paths with spaces in. I didn't realize my system() change was the cause of that breakage.
spawn that doesn't block Geany or outright hang. This definitely
It would be nice to figure out *why* it was hanging and fix *that* problem though, not just drop the existing workaround code and write 3rd workaround that's arguably worse[1] than the original issue.
I did try.
happened very frequently when using dmd from dlang.org. See:
https://github.com/geany/geany/commit/a3664fae9ece396952d732cc937e63192d8c6f...
I don't remember seeing any bug reports for this, so it's possible it's limited to you or the specific application you were using, maybe a bug in a certain version of DMD? The commit messages doesn't explain what the actual root of the problem was or why it could not be fixed properly, or what was tried to fix it before abandoning the existing code.
It was last year, but I think it wasn't just with dmd, although that triggered it the most, and it only happened when dmd wrote more than a certain amount to stderr (cascading errors). It was very suspicious that the custom spawn code works until there are too many errors, at least with dmd.
BTW the blocking and hanging was with the custom windows process spawning code, not with GLib's spawning. GLib IME just doesn't report the output properly, or at least not both stdout and stderr for all binaries.
What did you try to fix it before dropping asynchronous spawning? How
I didn't drop it, Enrico/contributor did some years ago.
did it improperly report its output? Did it cause the handlers for the IO channels not to always fire or something? What programs didn't it work with? What versions were tested? Can it be reproduced in a clean sample program outside of Geany?
BTW, do you have a link to the upstream bug report for this issue? I googled a lot, and besides this old fixed issue[2], I don't seem to find much besides a few Geany-related things that just say "don't work" without additional details. Did anyone from Geany report the bug upstream or make note of which upstream bug is causing the original original issue?
I don't have any links, but there are many serious sounding bugs in Glib's bug tracker for g_spawn on Windows.
Cheers, Matthew Brush
[1]: Causes a command prompt to popup, doesn't support filenames with spaces, uses a temp file, uses system(), leaves a bunch of complicated dead code, now 3rd way to spawn process, etc. [2]: https://bugzilla.gnome.org/show_bug.cgi?id=510664
On 05/10/2013 14:59, Nick Treleaven wrote:
spawn that doesn't block Geany or outright hang. This definitely
It would be nice to figure out *why* it was hanging and fix *that* problem though, not just drop the existing workaround code and write 3rd workaround that's arguably worse[1] than the original issue.
I did try.
In fact I wasted about a week trying to fix it, reading windows API docs, reading forum threads etc.
happened very frequently when using dmd from dlang.org. See:
https://github.com/geany/geany/commit/a3664fae9ece396952d732cc937e63192d8c6f...
I don't remember seeing any bug reports for this, so it's possible it's limited to you or the specific application you were using, maybe a bug in a certain version of DMD? The commit messages doesn't explain what the actual root of the problem was or why it could not be fixed properly, or what was tried to fix it before abandoning the existing code.
It was last year, but I think it wasn't just with dmd, although that triggered it the most, and it only happened when dmd wrote more than a certain amount to stderr (cascading errors). It was very suspicious that the custom spawn code works until there are too many errors, at least with dmd.
One person reported it on the devel list. See: http://lists.geany.org/pipermail/devel/2011-July/004845.html
Unfortunately I missed that mail at the time, but I tried the provided fix before committing my system() change, and I remember it didn't fix all the issues with blocking and/or hanging Geany I had.
On 13-10-05 07:06 AM, Nick Treleaven wrote:
On 05/10/2013 14:59, Nick Treleaven wrote:
spawn that doesn't block Geany or outright hang. This definitely
It would be nice to figure out *why* it was hanging and fix *that* problem though, not just drop the existing workaround code and write 3rd workaround that's arguably worse[1] than the original issue.
I did try.
In fact I wasted about a week trying to fix it, reading windows API docs, reading forum threads etc.
Ugh, been there, I feel for ya :)
happened very frequently when using dmd from dlang.org. See:
https://github.com/geany/geany/commit/a3664fae9ece396952d732cc937e63192d8c6f...
I don't remember seeing any bug reports for this, so it's possible it's limited to you or the specific application you were using, maybe a bug in a certain version of DMD? The commit messages doesn't explain what the actual root of the problem was or why it could not be fixed properly, or what was tried to fix it before abandoning the existing code.
It was last year, but I think it wasn't just with dmd, although that triggered it the most, and it only happened when dmd wrote more than a certain amount to stderr (cascading errors). It was very suspicious that the custom spawn code works until there are too many errors, at least with dmd.
One person reported it on the devel list. See: http://lists.geany.org/pipermail/devel/2011-July/004845.html
Unfortunately I missed that mail at the time, but I tried the provided fix before committing my system() change, and I remember it didn't fix all the issues with blocking and/or hanging Geany I had.
That was going to be my guess for the deadlock, once the buffers fill up they have to be drained or else can't keep writing to input. The same issue should apply to any spawning method we use probably (ie. GSpawn).
So with the pull request adding system() as a fallback/hidden preference, maybe we could just drop the win32 API code altogether, switch back to using the same codepath as all platforms (ie. GLib async spawning) and if people experience issues, we can use as further data points to troubleshoot GLib async spawning and we can recommend they try the system() option[1] as a workaround?
IMO, any function in our code base prefixed with _broken, that no one wants to touch, and looses platform-independence for our code, should just be deleted[2] :)
BTW, did you try undefining SYNC_SPAWN lately to see if async spawning still doesn't work for you now? If so and it doesn't, what Windows version and GLib versions do you have?
Cheers, Matthew Brush
[1]: And maybe fix it so it does cmd.exe quoting properly. [2]: Of course it's in Git history so if async spawning ends up not working for many people we can always put it back later.
On Sat, 05 Oct 2013 07:40:15 -0700 Matthew Brush mbrush@codebrainz.ca wrote:
So with the pull request adding system() as a fallback/hidden preference, maybe we could just drop the win32 API code altogether, switch back to using the same codepath as all platforms (ie. GLib async spawning) and if people experience issues, we can use as further data points to troubleshoot GLib async spawning and we can recommend they try the system() option[1] as a workaround?
Scope uses g_spawn_async_with_pipes() on Win~1 to both read from and write to gdb without any problems that I know of. Could be because of the specially written event source, or simply my ignorance. Also can't remember whether I specially tested stderr.
On 6 October 2013 06:06, Dimitar Zhekov dimitar.zhekov@gmail.com wrote:
On Sat, 05 Oct 2013 07:40:15 -0700 Matthew Brush mbrush@codebrainz.ca wrote:
So with the pull request adding system() as a fallback/hidden preference, maybe we could just drop the win32 API code altogether, switch back to using the same codepath as all platforms (ie. GLib async spawning) and if people experience issues, we can use as further data points to troubleshoot GLib async spawning and we can recommend they try the system() option[1] as a workaround?
Scope uses g_spawn_async_with_pipes() on Win~1 to both read from and write to gdb without any problems that I know of. Could be because of the specially written event source, or simply my ignorance. Also can't remember whether I specially tested stderr.
Or possibly works because its just short messages, IIRC one of the problems only showed up with lots of data.
Cheers Lex
-- E-gards: Jimmy _______________________________________________ Devel mailing list Devel@lists.geany.org https://lists.geany.org/cgi-bin/mailman/listinfo/devel
On Sun, 6 Oct 2013 11:02:07 +1100 Lex Trotman elextr@gmail.com wrote:
On 6 October 2013 06:06, Dimitar Zhekov dimitar.zhekov@gmail.com wrote:
On Sat, 05 Oct 2013 07:40:15 -0700 Matthew Brush mbrush@codebrainz.ca wrote:
So with the pull request adding system() as a fallback/hidden preference, maybe we could just drop the win32 API code altogether, switch back to using the same codepath as all platforms (ie. GLib async spawning) and if people experience issues, we can use as further data points to troubleshoot GLib async spawning and we can recommend they try the system() option[1] as a workaround?
Scope uses g_spawn_async_with_pipes() on Win~1 to both read from and write to gdb without any problems that I know of. Could be because of the specially written event source, or simply my ignorance. Also can't remember whether I specially tested stderr.
Or possibly works because its just short messages, IIRC one of the problems only showed up with lots of data.
Unlikely. It's fully async, and even reentrable for the most part, so the message size should not matter. And since the win~1 was custom, I tested the *** thing with the order of megabytes while developing it. BTW, I re-checked stderr, and it works fine.
On 13-10-07 09:13 AM, Dimitar Zhekov wrote:
On Sun, 6 Oct 2013 11:02:07 +1100 Lex Trotman elextr@gmail.com wrote:
On 6 October 2013 06:06, Dimitar Zhekov dimitar.zhekov@gmail.com wrote:
On Sat, 05 Oct 2013 07:40:15 -0700 Matthew Brush mbrush@codebrainz.ca wrote:
So with the pull request adding system() as a fallback/hidden preference, maybe we could just drop the win32 API code altogether, switch back to using the same codepath as all platforms (ie. GLib async spawning) and if people experience issues, we can use as further data points to troubleshoot GLib async spawning and we can recommend they try the system() option[1] as a workaround?
Scope uses g_spawn_async_with_pipes() on Win~1 to both read from and write to gdb without any problems that I know of. Could be because of the specially written event source, or simply my ignorance. Also can't remember whether I specially tested stderr.
Or possibly works because its just short messages, IIRC one of the problems only showed up with lots of data.
Unlikely. It's fully async, and even reentrable for the most part, so the message size should not matter. And since the win~1 was custom, I tested the *** thing with the order of megabytes while developing it. BTW, I re-checked stderr, and it works fine.
Any chance you could find time to review and test Geany's implementation to see if anything looks fishy?
OT: Do you have any idea how to make g_spawn_async_with_pipes() to not open a "Command Prompt" window on Windows? My new plugin filters the document text through a subprocess very often and the popup console windows are super annoying :)
Cheers, Matthew Brush
On Mon, 07 Oct 2013 12:28:49 -0700 Matthew Brush mbrush@codebrainz.ca wrote:
[...]
Any chance you could find time to review and test Geany's implementation to see if anything looks fishy?
I can simply test FiF with regex = ".", which will generate a lot of output quite fast. Without the SYNC_SPAWN patches, build.c and search.c async executions are identical (except that build checks if the returned pid after successful spawn async is > 0, but still sets the channels if it's not - I wonder why).
GTK+ 2.22 is over 3 years old by now, FWIW.
I'm using 2.22 under win~1 too, and don't think it will have any problems with async exec, but we'll see.
OT: Do you have any idea how to make g_spawn_async_with_pipes() to not open a "Command Prompt" window on Windows? My new plugin filters the document text through a subprocess very often and the popup console windows are super annoying :)
A win~1 .exe header contains a value which indicates whether it's GUI or console (or something else). For any console executable, win~1 will open a console automatically, unless one already exists (i.e. running grep from cmd will not open a new console). It's not really a "command prompt", unless you start it with system() or via cmd.
What you can do is run the executable in a mimimized [console] window. Creating a shortcut with "window = minimized" may affect the execution even if you run the .exe instead of the .pif or .lnk, and if it doesn't, you can try to run the shortcut, but I'm not sure the CLI arguments will work properly that way.
Last, you can create a small GUI subsystem wrapper which runs your console application with CreateProcess(), and use either the creation flags or the startup information to hide or minimize it.
On 13-10-08 10:28 AM, Dimitar Zhekov wrote:
On Mon, 07 Oct 2013 12:28:49 -0700 Matthew Brush mbrush@codebrainz.ca wrote:
[...]
Any chance you could find time to review and test Geany's implementation to see if anything looks fishy?
I can simply test FiF with regex = ".", which will generate a lot of output quite fast. Without the SYNC_SPAWN patches, build.c and search.c async executions are identical (except that build checks if the returned pid after successful spawn async is > 0, but still sets the channels if it's not - I wonder why).
Can you get grep to write a lot of stuff to both stdout and stderr in the same run though? What about wrapping grep in a shell script that uses `tee` (or equivalent) to dump the output to both streams?
GTK+ 2.22 is over 3 years old by now, FWIW.
I'm using 2.22 under win~1 too, and don't think it will have any problems with async exec, but we'll see.
Yeah, just pointing it out because 3 years is a lot of code churn for a project like GTK+. There's been ~15.5 thousand commits[1] since the 2.22.0 tag :)
OT: Do you have any idea how to make g_spawn_async_with_pipes() to not open a "Command Prompt" window on Windows? My new plugin filters the document text through a subprocess very often and the popup console windows are super annoying :)
[snip]
Last, you can create a small GUI subsystem wrapper which runs your console application with CreateProcess(), and use either the creation flags or the startup information to hide or minimize it.
Hmm, this is probably the best and easiest solution, thanks for the idea!
Cheers, Matthew Brush
[1]: According to `git rev-list 2.22.0^..HEAD | wc -l`, not sure if that's the right way, my gitfu is weak.
On 09/10/2013 01:50, Matthew Brush wrote:
I can simply test FiF with regex = ".", which will generate a lot of output quite fast. Without the SYNC_SPAWN patches, build.c and search.c async executions are identical (except that build checks if the returned pid after successful spawn async is > 0, but still sets the channels if it's not - I wonder why).
Can you get grep to write a lot of stuff to both stdout and stderr in the same run though?
Exactly, the problem with g_spawn is not the 4kb problem, that is only the custom spawning. The problem is capturing both stdout and stderr.
What about wrapping grep in a shell script that uses `tee` (or equivalent) to dump the output to both streams?
GTK+ 2.22 is over 3 years old by now, FWIW.
I'm using 2.22 under win~1 too, and don't think it will have any problems with async exec, but we'll see.
Yeah, just pointing it out because 3 years is a lot of code churn for a project like GTK+. There's been ~15.5 thousand commits[1] since the 2.22.0 tag :)
g_spawn isn't gtk. gspawn[,-win,-win-helper].c don't seem to have any significant fixes. I could be wrong, but it doesn't look hopeful.
On 13-10-09 07:42 AM, Nick Treleaven wrote:
On 09/10/2013 01:50, Matthew Brush wrote:
[snip]
GTK+ 2.22 is over 3 years old by now, FWIW.
I'm using 2.22 under win~1 too, and don't think it will have any problems with async exec, but we'll see.
Yeah, just pointing it out because 3 years is a lot of code churn for a project like GTK+. There's been ~15.5 thousand commits[1] since the 2.22.0 tag :)
g_spawn isn't gtk. gspawn[,-win,-win-helper].c don't seem to have any significant fixes. I could be wrong, but it doesn't look hopeful.
Yeah, sorry, I was speaking more generally (ie. OT) about using an older GTK+ bundle with there being so many changes in each GTK+ release, not specifically about the thread topic/GSpawn stuff.
Cheers, Matthew Brush
Hi,
I don't have today, so the results quickly. Short test proram, which writes "The quick brown fox jumps over the stdout/err %d times\n", plus a few characters in stdout to create "valid" gdb output.
Geany, with 10000 iterations: dies at stdout 140, stderr 128. I cancelled it after 2 minutes, then after 30 minutes - same thing. Disheartening.
With 100000 iterations (12.28MB of text):
[C:] emit >& __emit: 1 second +-0.5
[C:] emit [console output]: 2 minutes flat
Scope, with a source_check() modified to handle stdout and stderr only (FiF/build do not write to process stdin): 14 seconds
Scope, without "\n" in stdout (converted to \n when reading gdb output), so the text is joined in GtkTextView-lines of few/several KB (depending on when stderr breaks them): 2 minutes 45 seconds
--
So, it is not the async execution itself that's broken. GIOChannel set_flags and/or add_watch are. Well, we already knew that GLib non-blocking I/O and poll are problematic with win~1 anonymous pipes, and discussed than on the ML. Strictly speaking, win~1 has async I/O with events, not poll, so GLib emulates it with a 2nd thread IIRC...
BTW, I forced stdout and stderr to real non-blocking I/O, and the result was quite strange: emit finishes for ~1 second, but Geany does not receive anything.
-- E-gards: Jimmy
Hi,
I'm looking how to fix this famous bug without too many changes, and found something strange: tools.c runs it's command with spawn async, but uses blocking I/O to read the data from stdout/stderr.
Any idea why is that? What comes to ming is blocking Geany until the command finishes, to be sure that the current document still exists and it's selection is unchanged, and thus can be safely replaced with the command output.
However, if we block reading stderr on Windows, and the command outputs 4KB (or whatever is the limit) to stdout, that'll cause a deadlock, as evident by the previous messages in this thread. Same if reading stdout and the command produces a lot of stderr, though that's unlikely.
I can emulate the (non-)blocking behaviour, but am not sure if this flag currently has any effect under Windows, considering it's buggy implementation of g_io_add_watch and g_io_channel_set_flags for pipes (which are the problem to fix in the first place).
If tools_execute_custom_command really wants to block Geany, the right thing is to spawn the command, and then cycle reading stderr and stdout asynchronously until the command completes, instead of adding I/O watches to the message loop just to block it.
The current loop that writes the selection to stdin seems capable of causing a deadlock under Windows too, if the command generates 4KB output and stops before the full selection is written, since we use blocking I/O to write to stdin...
On 15 October 2013 03:15, Dimitar Zhekov dimitar.zhekov@gmail.com wrote:
Hi,
I'm looking how to fix this famous bug without too many changes, and found something strange: tools.c runs it's command with spawn async, but uses blocking I/O to read the data from stdout/stderr.
Given that it sets up io channels and callbacks when data is available I suspect that the writer thought they were using asynchronous i/o. ;)
Any idea why is that? What comes to ming is blocking Geany until the command finishes, to be sure that the current document still exists and it's selection is unchanged, and thus can be safely replaced with the command output.
This would be a correct reason to block Geany, but deliberately, not by accident.
However, if we block reading stderr on Windows, and the command outputs 4KB (or whatever is the limit) to stdout, that'll cause a deadlock, as evident by the previous messages in this thread. Same if reading stdout and the command produces a lot of stderr, though that's unlikely.
Its likely to do the same on Linux, but IIUC the limit is larger these days, 64k = 16 * PIPE_BUF. (The 16 is a #define in the kernal)
I can emulate the (non-)blocking behaviour, but am not sure if this flag currently has any effect under Windows, considering it's buggy implementation of g_io_add_watch and g_io_channel_set_flags for pipes (which are the problem to fix in the first place).
Or possibly try the more complicated GIO g_input_stream_read_async() and friends and see if it works on windows.
If tools_execute_custom_command really wants to block Geany, the right thing is to spawn the command, and then cycle reading stderr and stdout asynchronously until the command completes, instead of adding I/O watches to the message loop just to block it.
The current loop that writes the selection to stdin seems capable of causing a deadlock under Windows too, if the command generates 4KB output and stops before the full selection is written, since we use blocking I/O to write to stdin...
And not just on windows, if the selection is a whole file it can easily exceed the Linux limit of 64k.
Basically the whole spawning thing needs to be looked at critically, especially those uses that do i/o to the child process. A working version of each permutation sync/async, reads/writes/both needs to be put in utils and all of Geany should then use that implementation and the callbacks use known good idioms.
Cheers Lex
-- E-gards: Jimmy _______________________________________________ Devel mailing list Devel@lists.geany.org https://lists.geany.org/cgi-bin/mailman/listinfo/devel
On Tue, 15 Oct 2013 11:29:17 +1100 Lex Trotman elextr@gmail.com wrote:
A good news first: using a bit of Scope code, I was able to pipe 12.3MB of mixed stdout and stderr output into Geany, in async mode, for just a few seconds.
There is a delay when scrolling the messages down, and the scroll bar gets updated - seems that GtkTreeView does not handle all lines until you come to them (depending on some options). Pretty reasonable.
Of course, there will be more tests, especially sync, before I post a patch to SF Bug #943.
Any idea why is that? What comes to ming is blocking Geany until the command finishes, to be sure that the current document still exists and it's selection is unchanged, and thus can be safely replaced with the command output.
This would be a correct reason to block Geany, but deliberately, not by accident.
tools.c does replace the selection without checking if the document is still valid and whether the selection has been changed. Working with a closed document may cause a crash, so it looks intentional.
I can emulate the (non-)blocking behaviour, but am not sure if this flag currently has any effect under Windows, considering it's buggy implementation of g_io_add_watch and g_io_channel_set_flags for pipes (which are the problem to fix in the first place).
Or possibly try the more complicated GIO g_input_stream_read_async() and friends and see if it works on windows.
The last time FiF-ed, glib did not contain any calls to PeekNamedPipe or SetNamedPipeHandleState. I am not aware of any way to implement async anonymous pipe I/O under Windows without these, and see no reason to check the various glib functions one by one. (Don't be fooled by "Named", these work for anonymous pipes, and in fact the win~1 named pipes don't need them as they support event based I/O).
If tools_execute_custom_command really wants to block Geany, the right thing is to spawn the command, and then cycle reading stderr and stdout asynchronously until the command completes, instead of adding I/O watches to the message loop just to block it.
The current loop that writes the selection to stdin seems capable of causing a deadlock under Windows too, if the command generates 4KB output and stops before the full selection is written, since we use blocking I/O to write to stdin...
And not just on windows, if the selection is a whole file it can easily exceed the Linux limit of 64k.
I was wondering if there is any limitation under *nix...
Basically the whole spawning thing needs to be looked at critically, especially those uses that do i/o to the child process. A working version of each permutation sync/async, reads/writes/both needs to be put in utils and all of Geany should then use that implementation and the callbacks use known good idioms.
Blocking the GTK+ message loop (aka Geany) to avoid document changes and using blocking pipe I/O are two different things, and we don't need the later AFAIK.
A good blocking without rewriting the current code is by displaying a small modal dialog "Running FOO..." with a Cancel button. The keyboard and mouse events are redirected to it, keeping the documents fair, and there's a way to stop a slow process which intentionally blocks Geany.
That being said, I agree we need something better than the current (a bit naive) implementations, and I know where to find it. :) Whether the leading developers will accept such changes is another matter.
Am 15.10.2013 19:05, schrieb Dimitar Zhekov:
The last time FiF-ed, glib did not contain any calls to PeekNamedPipe or SetNamedPipeHandleState. I am not aware of any way to implement async anonymous pipe I/O under Windows without these, and see no reason to check the various glib functions one by one. (Don't be fooled by "Named", these work for anonymous pipes, and in fact the win~1 named pipes don't need them as they support event based I/O).
Speaking of PeekNamedPipe...
A few months ago I contributed a fully working win32 poll() implementation to PulseAudio (based on gnulib). Because pacat could not read from a process that outputs an audio data stream to stdout, and this case did not work with their poll() function (it really only handled sockets properly). Anyway, the poll() I provided handles pipes and console stdin/stdout as well. You can find it here: http://cgit.freedesktop.org/pulseaudio/pulseaudio/tree/src/pulsecore/poll-wi...
I remember I looked how glib implements poll() on Windows (because pulseaudio still failed when it used its glib-mainloop plugin) and it also didn't have a proper implementation. I think glib uses poll() for GIO stuff and based on my findings I would expect that glib does poorly with GIO on win32 pipes.
Perhaps it would be worthwhile to get that poll() implementation into glib?
Best regards.
On Tue, 15 Oct 2013 22:02:45 +0200 Thomas Martitz thomas.martitz@student.htw-berlin.de wrote:
Am 15.10.2013 19:05, schrieb Dimitar Zhekov:
The last time FiF-ed, glib did not contain any calls to PeekNamedPipe or SetNamedPipeHandleState. [...]
Speaking of PeekNamedPipe...
A few months ago I contributed a fully working win32 poll() implementation to PulseAudio (based on gnulib). [...]
Yes, I remember that.
Perhaps it would be worthwhile to get that poll() implementation into glib?
You can simply g_main_context_set_poll_func(), but that so called "poll" function actually supports the WIN32 message queue, and maybe some other stuff... Of course, it would be nice to at least have a proper GPoll.
On 16 October 2013 04:05, Dimitar Zhekov dimitar.zhekov@gmail.com wrote:
On Tue, 15 Oct 2013 11:29:17 +1100 Lex Trotman elextr@gmail.com wrote:
A good news first: using a bit of Scope code, I was able to pipe 12.3MB of mixed stdout and stderr output into Geany, in async mode, for just a few seconds.
There is a delay when scrolling the messages down, and the scroll bar gets updated - seems that GtkTreeView does not handle all lines until you come to them (depending on some options). Pretty reasonable.
Of course, there will be more tests, especially sync, before I post a patch to SF Bug #943.
That sounds encouraging, maybe post a PR and just refer to it from the bug.
[...] tools.c does replace the selection without checking if the document is still valid and whether the selection has been changed. Working with a closed document may cause a crash, so it looks intentional.
Sounds like it should use a synchronous spawn.
[...]
The last time FiF-ed, glib did not contain any calls to PeekNamedPipe or SetNamedPipeHandleState. I am not aware of any way to implement async anonymous pipe I/O under Windows without these, and see no reason to check the various glib functions one by one. (Don't be fooled by "Named", these work for anonymous pipes, and in fact the win~1 named pipes don't need them as they support event based I/O).
Ok, and Thomas supports you there.
[...]
Basically the whole spawning thing needs to be looked at critically, especially those uses that do i/o to the child process. A working
version
of each permutation sync/async, reads/writes/both needs to be put in
utils
and all of Geany should then use that implementation and the callbacks
use
known good idioms.
Blocking the GTK+ message loop (aka Geany) to avoid document changes and using blocking pipe I/O are two different things, and we don't need the later AFAIK.
Agree, by sync (synchronous) we tend to mean blocking Geany until finished. By async we mean not doing so. As you say it would be better to achieve it without blocking i/o because of the deadlock risks. Anything that is going to modify the buffer should be synchronous, things like build should be asynchronous.
A good blocking without rewriting the current code is by displaying a small modal dialog "Running FOO..." with a Cancel button. The keyboard and mouse events are redirected to it, keeping the documents fair, and there's a way to stop a slow process which intentionally blocks Geany.
Thats a possibility, though some people would find them annoying. But having their output work correctly is more important :)
That being said, I agree we need something better than the current (a bit naive) implementations, and I know where to find it. :) Whether the leading developers will accept such changes is another matter.
For me, you will be *very* welcome if you can provide a proper set of multi-platform spawn with/without stdin/stdout/stderr that can remove the #ifdef G_OS_WIN32 stuff from build.c that I have been tiptoeing around for years not daring to pass the mouse over it in case it broke (worse). :)
@Thomas,
Sounds like a useful patch, definitely pass it to glib, I'm sure the windows support guy(s?) will be glad of help. In the mean time is it licensed suitably for Dimitar to steal?
Cheers Lex
-- E-gards: Jimmy _______________________________________________ Devel mailing list Devel@lists.geany.org https://lists.geany.org/cgi-bin/mailman/listinfo/devel
Am 16.10.2013 01:52, schrieb Lex Trotman:
For me, you will be *very* welcome if you can provide a proper set of multi-platform spawn with/without stdin/stdout/stderr that can remove the #ifdef G_OS_WIN32 stuff from build.c that I have been tiptoeing around for years not daring to pass the mouse over it in case it broke (worse). :)
@Thomas,
Sounds like a useful patch, definitely pass it to glib, I'm sure the windows support guy(s?) will be glad of help. In the mean time is it licensed suitably for Dimitar to steal?
I will see. It's LGPLv2+ so should be fine.
Best regards.
On Wed, 16 Oct 2013 10:52:33 +1100 Lex Trotman elextr@gmail.com wrote:
On 16 October 2013 04:05, Dimitar Zhekov dimitar.zhekov@gmail.com wrote:
A good news first: using a bit of Scope code, I was able to pipe 12.3MB of mixed stdout and stderr output into Geany, in async mode, for just a few seconds. [...]
Of course, there will be more tests, especially sync, before I post a patch to SF Bug #943.
That sounds encouraging, maybe post a PR and just refer to it from the bug.
Actually, I finished most of it, what remains is mainly blocking Geany while the new async tools execution is running.
Sync spawning is not good, because I'm using the Geany message loop for async I/O (somewhat similar to g_main_context_add_poll), not real Win32 events and threads as GLib does.
Locking the file, as Matthew suggested, will not be sufficient: one can still close it, reload it, and of course change the selection (SCI docs assume close/reload are be blocked for background saving).
What remains, without going too deep, is a modal dialog. Annoying, of course, but with the benefit that you can easily stop a bad process.
That being said, I agree we need something better than the current (a bit naive) implementations, and I know where to find it. :) Whether the leading developers will accept such changes is another matter.
For me, you will be *very* welcome if you can provide a proper set of multi-platform spawn with/without stdin/stdout/stderr that can remove the #ifdef G_OS_WIN32 stuff from build.c that I have been tiptoeing around for years not daring to pass the mouse over it in case it broke (worse). :)
For now, I'll concentrate on working build, tools and search.
Of course, with a proper win~1 blocking I/O, tools deadlocked, even faster that the sync spawn in build. Another bug in tools is that, when writing to the process stdin, it counts the remaining bytes, but always write()-s from the beginning of the selection. :)
As of a rewrite, maybe after we have at least working GPoll in glib, it would be much easier. That's more about organizing the read-error / write-error / process-exit events properly than platform compatibility. For example, currently tools waits for read stderr to return something other than G_IO_STATUS_NORMAL or G_IO_STATUS_AGAIN fail to decide that it's time to replace the selection (or skip the replacement, if there were any error messages). That has some merit, there may be buffered messages after the process dies, but is not the best thing.
Don't worry about the build SYNC spawn, I'll kill it in any case.
Am 16.10.2013 21:04, schrieb Dimitar Zhekov:
Actually, I finished most of it, what remains is mainly blocking Geany while the new async tools execution is running.
Sync spawning is not good, because I'm using the Geany message loop for async I/O (somewhat similar to g_main_context_add_poll), not real Win32 events and threads as GLib does.
Locking the file, as Matthew suggested, will not be sufficient: one can still close it, reload it, and of course change the selection (SCI docs assume close/reload are be blocked for background saving).
What remains, without going too deep, is a modal dialog. Annoying, of course, but with the benefit that you can easily stop a bad process.
I may have missed it but why have it blocking if async spawn now works? The current build process (on Linux) doesn't block either.
Best regards
On 17 October 2013 06:08, Thomas Martitz < thomas.martitz@student.htw-berlin.de> wrote:
Am 16.10.2013 21:04, schrieb Dimitar Zhekov:
Actually, I finished most of it, what remains is mainly blocking Geany while the new async tools execution is running.
Sync spawning is not good, because I'm using the Geany message loop for async I/O (somewhat similar to g_main_context_add_poll), not real Win32 events and threads as GLib does.
Locking the file, as Matthew suggested, will not be sufficient: one can still close it, reload it, and of course change the selection (SCI docs assume close/reload are be blocked for background saving).
What remains, without going too deep, is a modal dialog. Annoying, of course, but with the benefit that you can easily stop a bad process.
I may have missed it but why have it blocking if async spawn now works? The current build process (on Linux) doesn't block either.
I think Dimitar is talking about for tools use where the results of the command are pasted to the buffer, so the existence of the buffer and the selection should not change while the command is running.
Cheers Lex
Best regards
______________________________**_________________ Devel mailing list Devel@lists.geany.org https://lists.geany.org/cgi-**bin/mailman/listinfo/develhttps://lists.geany.org/cgi-bin/mailman/listinfo/devel
On Thu, 17 Oct 2013 10:04:11 +1100 Lex Trotman elextr@gmail.com wrote:
On 17 October 2013 06:08, Thomas Martitz < thomas.martitz@student.htw-berlin.de> wrote:
Am 16.10.2013 21:04, schrieb Dimitar Zhekov:
What remains, without going too deep, is a modal dialog. Annoying, of course, but with the benefit that you can easily stop a bad process.
I may have missed it but why have it blocking if async spawn now works? The current build process (on Linux) doesn't block either.
I think Dimitar is talking about for tools use where the results of the command are pasted to the buffer, so the existence of the buffer and the selection should not change while the command is running.
Exactly.
I'm a bit sick and overworked now, so here's an incomplete patch. When sending the current selection to a custom command, please don't close or modify the document - best of all, wait for the command to complete. Everything else should work, though it's not pretty.
The utils_set_up_io_channel() macro will be removed, and utils_setup_io_channel() will remain, without cond (::= G_IO_IN | G_IO_PRI | G_IO_ERR | G_IO_HUP | G_IO_NVAL) and nblock (::= TRUE).
I don't care about the names of the new structure and functions. Once this thing is complete and working, rename them as you see fit.
On Thu, 17 Oct 2013 10:04:11 +1100 Lex Trotman elextr@gmail.com wrote:
On 17 October 2013 06:08, Thomas Martitz < thomas.martitz@student.htw-berlin.de> wrote:
Am 16.10.2013 21:04, schrieb Dimitar Zhekov:
What remains, without going too deep, is a modal dialog. Annoying, of course, but with the benefit that you can easily stop a bad process.
I may have missed it but why have it blocking if async spawn now works? The current build process (on Linux) doesn't block either.
I think Dimitar is talking about for tools use where the results of the command are pasted to the buffer, so the existence of the buffer and the selection should not change while the command is running.
Exactly.
I'm a bit sick and overworked now, so here's an incomplete patch. When sending the current selection to a custom command, please don't close or modify the document - best of all, wait for the command to complete. Everything else should work, though it's not pretty.
The utils_set_up_io_channel() macro will be removed, and utils_setup_io_channel() will remain, without cond (::= G_IO_IN | G_IO_PRI | G_IO_ERR | G_IO_HUP | G_IO_NVAL) and nblock (::= TRUE).
I don't care about the names of the new structure and functions. Once this thing is complete and working, rename them as you see fit.
On 13-10-16 12:04 PM, Dimitar Zhekov wrote:
[snip]
Actually, I finished most of it, what remains is mainly blocking Geany while the new async tools execution is running.
Sync spawning is not good, because I'm using the Geany message loop for async I/O (somewhat similar to g_main_context_add_poll), not real Win32 events and threads as GLib does.
Locking the file, as Matthew suggested, will not be sufficient: one can still close it, reload it, and of course change the selection (SCI docs assume close/reload are be blocked for background saving).
Yeah, in a program like Geany where it doesn't use Actions to make it easy to control the multiple widgets that represent the same action, it's certainly less trivial. That's why I said "Assuming re-writing more of the code isn't actually a bad thing..." :) For example, if Geany was made to use actions, you could disable closing and reloading as simple as two calls like:
gtk_action_set_sensitive(actionClose, false); gtk_action_set_sensitive(actionReload, false);
or some document-specific variation of that. In the case of my little editor I was writing, I had the Qt actions and also the translucent overlay (mentioned but not shown) which prevented the user from interacting with the Scintilla view while locked.
Cheers, Matthew Brush
On 13-10-15 10:05 AM, Dimitar Zhekov wrote:
On Tue, 15 Oct 2013 11:29:17 +1100 Lex Trotman elextr@gmail.com wrote:
[snip]
Basically the whole spawning thing needs to be looked at critically, especially those uses that do i/o to the child process. A working version of each permutation sync/async, reads/writes/both needs to be put in utils and all of Geany should then use that implementation and the callbacks use known good idioms.
Blocking the GTK+ message loop (aka Geany) to avoid document changes and using blocking pipe I/O are two different things, and we don't need the later AFAIK.
A good blocking without rewriting the current code is by displaying a small modal dialog "Running FOO..." with a Cancel button. The keyboard and mouse events are redirected to it, keeping the documents fair, and there's a way to stop a slow process which intentionally blocks Geany.
Assuming re-writing more of the code isn't actually a bad thing, a good way for Scintilla is the way recommended in the docs[1] for background saving/loading, and I think you said Scope does it too, is to just make the affected document(s) read-only while the task executes.
I did something quite similar in an editor I was working on a while I ago and still have a video on my site[1] that shows a possible UI for something like this. The actual program also dimmed the document with a semi-transparent black overlay over the document and underneath the progress bar to make it more obvious that that particular document was locked, but it wasn't implemented when the video was recorded.
Cheers, Matthew Brush
[1]: http://codebrainz.ca/screencasts/Loading300MBFile.avi [2]: http://www.scintilla.org/ScintillaDoc.html#BackgroundLoadSave
On Tue, 08 Oct 2013 17:50:15 -0700 Matthew Brush mbrush@codebrainz.ca wrote:
On 13-10-08 10:28 AM, Dimitar Zhekov wrote:
On Mon, 07 Oct 2013 12:28:49 -0700 Matthew Brush mbrush@codebrainz.ca wrote:
[...]
Any chance you could find time to review and test Geany's implementation to see if anything looks fishy?
I can simply test FiF with regex = ".", which will generate a lot of output quite fast [...]
Can you get grep to write a lot of stuff to both stdout and stderr in the same run though? What about wrapping grep in a shell script that uses `tee` (or equivalent) to dump the output to both streams?
I can, but it would be easier to write a small program tham emits to stdio and stderr, that's what I did for Scope (along with sending a lot of data to the program, but it'll be useless in our case).
For now: grep has no problem searching C:...\include with subdirectories with ~235K matches, and without the subdirectories with ~77K matches and some error messages "coulnd not open <subdir>". The recursive match count was not 100% identical to running grep from CLI, probably because I tried to reproduce the CLI command in FiF instead of vice versa, and that turned out to be impossible. I was never quite happy with FiF, and perhaps the time has come for a thread how to improve it a bit...
On 05/10/2013 15:40, Matthew Brush wrote:
On 13-10-05 07:06 AM, Nick Treleaven wrote:
On 05/10/2013 14:59, Nick Treleaven wrote:
I don't remember seeing any bug reports for this, so it's possible it's limited to you or the specific application you were using, maybe a bug in a certain version of DMD? The commit messages doesn't explain what the actual root of the problem was or why it could not be fixed properly, or what was tried to fix it before abandoning the existing code.
It was last year, but I think it wasn't just with dmd, although that triggered it the most, and it only happened when dmd wrote more than a certain amount to stderr (cascading errors). It was very suspicious that the custom spawn code works until there are too many errors, at least with dmd.
One person reported it on the devel list. See: http://lists.geany.org/pipermail/devel/2011-July/004845.html
Unfortunately I missed that mail at the time, but I tried the provided fix before committing my system() change, and I remember it didn't fix all the issues with blocking and/or hanging Geany I had.
That was going to be my guess for the deadlock, once the buffers fill up they have to be drained or else can't keep writing to input. The same issue should apply to any spawning method we use probably (ie. GSpawn).
Did we get deadlock with g_spawn? I'm only aware of missing output.
So with the pull request adding system() as a fallback/hidden preference, maybe we could just drop the win32 API code altogether, switch back to using the same codepath as all platforms (ie. GLib async spawning) and if people experience issues, we can use as further data points to troubleshoot GLib async spawning and we can recommend they try the system() option[1] as a workaround?
system() is *synchronous*, so we couldn't remove the SYNC_SPAWN code and have it as an option.
IMO, any function in our code base prefixed with _broken, that no one wants to touch, and looses platform-independence for our code, should just be deleted[2] :)
I thought you said it was most important to fix the common case, that's why I made the PR.
BTW, did you try undefining SYNC_SPAWN lately to see if async spawning still doesn't work for you now? If so and it doesn't, what Windows version and GLib versions do you have?
I just tried it. Running build on a C file:
gcc -Wall -c "build.c" (in directory: C:\git\geany\src) Compilation failed. gcc: error: "build.c": Invalid argument gcc: fatal error: no input files compilation terminated.
No idea why gcc says that, also tried for another file in parent directory.
Running build on a D file just says 'Compilation failed' without reporting the line compilation fails on.
Windows Vista. GTK 2.22.0, GLib 2.26.0.
Also if you want to make the argument that it might be fixed in newer versions, I would appreciate if you quoted something plausible that got fixed upstream in Git or in the release notes. To my knowledge no significant work has been done on it in years. See:
https://git.gnome.org/browse/glib/log/glib/gspawn-win32.c https://git.gnome.org/browse/glib/log/glib/gspawn-win32-helper.c
Cheers, Matthew Brush
[1]: And maybe fix it so it does cmd.exe quoting properly.
Feel free to look at it.
[2]: Of course it's in Git history so if async spawning ends up not working for many people we can always put it back later. _______________________________________________ Devel mailing list Devel@lists.geany.org https://lists.geany.org/cgi-bin/mailman/listinfo/devel
On 07/10/2013 16:31, Nick Treleaven wrote:
gcc -Wall -c "build.c" (in directory: C:\git\geany\src) Compilation failed. gcc: error: "build.c": Invalid argument gcc: fatal error: no input files compilation terminated.
No idea why gcc says that, also tried for another file in parent directory.
It was passing the quotes directly to gcc. Removed them, and it appears to work, but I think gcc only outputs to stderr.
I tried make build.o - the stdout gets captured but the stderr is lost.
On 13-10-07 08:31 AM, Nick Treleaven wrote:
On 05/10/2013 15:40, Matthew Brush wrote:
On 13-10-05 07:06 AM, Nick Treleaven wrote:
On 05/10/2013 14:59, Nick Treleaven wrote:
I don't remember seeing any bug reports for this, so it's possible it's limited to you or the specific application you were using, maybe a bug in a certain version of DMD? The commit messages doesn't explain what the actual root of the problem was or why it could not be fixed properly, or what was tried to fix it before abandoning the existing code.
It was last year, but I think it wasn't just with dmd, although that triggered it the most, and it only happened when dmd wrote more than a certain amount to stderr (cascading errors). It was very suspicious that the custom spawn code works until there are too many errors, at least with dmd.
One person reported it on the devel list. See: http://lists.geany.org/pipermail/devel/2011-July/004845.html
Unfortunately I missed that mail at the time, but I tried the provided fix before committing my system() change, and I remember it didn't fix all the issues with blocking and/or hanging Geany I had.
That was going to be my guess for the deadlock, once the buffers fill up they have to be drained or else can't keep writing to input. The same issue should apply to any spawning method we use probably (ie. GSpawn).
Did we get deadlock with g_spawn? I'm only aware of missing output.
It sounded like what was described by Josh in your link:
"The problem seems to be that pipes are only 4kb buffered by default, so any process needing to write more than this would block while writing to stdout and Geany would kill it, as it wasn't reading the pipes until after the process exited."
So with the pull request adding system() as a fallback/hidden preference, maybe we could just drop the win32 API code altogether, switch back to using the same codepath as all platforms (ie. GLib async spawning) and if people experience issues, we can use as further data points to troubleshoot GLib async spawning and we can recommend they try the system() option[1] as a workaround?
system() is *synchronous*, so we couldn't remove the SYNC_SPAWN code and have it as an option.
I mean to remove the win32_spawn function (as it existed before the system() hack), the one that was left orphaned in win32.c and renamed it to broken_win32_spawn() that uses the win32 API rather than GLib spawning or system().
IMO, any function in our code base prefixed with _broken, that no one wants to touch, and looses platform-independence for our code, should just be deleted[2] :)
I thought you said it was most important to fix the common case, that's why I made the PR.
I imagine the most common case would be that g_spawn_async_with_pipes() works. I've used it a number of times on Windows without issues and I've not found online or encountered any issues with lost output that's being claimed as the reason for all these work-arounds for it.
BTW, did you try undefining SYNC_SPAWN lately to see if async spawning still doesn't work for you now? If so and it doesn't, what Windows version and GLib versions do you have?
I just tried it. Running build on a C file:
gcc -Wall -c "build.c" (in directory: C:\git\geany\src) Compilation failed. gcc: error: "build.c": Invalid argument gcc: fatal error: no input files compilation terminated.
No idea why gcc says that, also tried for another file in parent directory.
Running build on a D file just says 'Compilation failed' without reporting the line compilation fails on.
Windows Vista. GTK 2.22.0, GLib 2.26.0.
GTK+ 2.22 is over 3 years old by now, FWIW.
Cheers, Matthew Brush
On 07/10/2013 20:07, Matthew Brush wrote:
On 13-10-07 08:31 AM, Nick Treleaven wrote:
On 05/10/2013 15:40, Matthew Brush wrote:
On 13-10-05 07:06 AM, Nick Treleaven wrote:
It was last year, but I think it wasn't just with dmd, although that triggered it the most, and it only happened when dmd wrote more than a certain amount to stderr (cascading errors). It was very suspicious that the custom spawn code works until there are too many errors, at least with dmd.
One person reported it on the devel list. See: http://lists.geany.org/pipermail/devel/2011-July/004845.html
Unfortunately I missed that mail at the time, but I tried the provided fix before committing my system() change, and I remember it didn't fix all the issues with blocking and/or hanging Geany I had.
That was going to be my guess for the deadlock, once the buffers fill up they have to be drained or else can't keep writing to input. The same issue should apply to any spawning method we use probably (ie. GSpawn).
Did we get deadlock with g_spawn? I'm only aware of missing output.
It sounded like what was described by Josh in your link:
"The problem seems to be that pipes are only 4kb buffered by default, so any process needing to write more than this would block while writing to stdout and Geany would kill it, as it wasn't reading the pipes until after the process exited."
But that link is talking about the custom spawning, and provides a patch to it. You must be confused, g_spawn shouldn't have that issue (only by coincidence, and I know of no evidence it has).
So with the pull request adding system() as a fallback/hidden preference, maybe we could just drop the win32 API code altogether, switch back to using the same codepath as all platforms (ie. GLib async spawning) and if people experience issues, we can use as further data points to troubleshoot GLib async spawning and we can recommend they try the system() option[1] as a workaround?
system() is *synchronous*, so we couldn't remove the SYNC_SPAWN code and have it as an option.
I mean to remove the win32_spawn function (as it existed before the system() hack), the one that was left orphaned in win32.c and renamed it to broken_win32_spawn() that uses the win32 API rather than GLib spawning or system().
It actually wasn't orphaned if env was set, but I get you.
IMO, any function in our code base prefixed with _broken, that no one wants to touch, and looses platform-independence for our code, should just be deleted[2] :)
I thought you said it was most important to fix the common case, that's why I made the PR.
I imagine the most common case would be that g_spawn_async_with_pipes() works. I've used it a number of times on Windows without issues and I've not found online or encountered any issues with lost output that's being claimed as the reason for all these work-arounds for it.
Can you please try running a command that outputs *both* stderr and stdout? See my reply to the parent message about make object and build.o.
BTW, did you try undefining SYNC_SPAWN lately to see if async spawning still doesn't work for you now? If so and it doesn't, what Windows version and GLib versions do you have?
I just tried it. Running build on a C file:
gcc -Wall -c "build.c" (in directory: C:\git\geany\src) Compilation failed. gcc: error: "build.c": Invalid argument gcc: fatal error: no input files compilation terminated.
No idea why gcc says that, also tried for another file in parent directory.
Running build on a D file just says 'Compilation failed' without reporting the line compilation fails on.
Windows Vista. GTK 2.22.0, GLib 2.26.0.
GTK+ 2.22 is over 3 years old by now, FWIW.
OK. Try to reproduce my problem with your newer gtk then.
On 05/10/2013 14:59, Nick Treleaven wrote:
Maybe to you, but it's very common on Windows to have spaces in the path names, it's even the default on Windows XP (Documents and Settings). To most people (ex. the bug reporters) it seems outright broken now, whereas before it worked.
See my other mail, I made a pull request.
New pull request: https://github.com/geany/geany/pull/180
This just fixes quoting the arguments so we can stick with the simple and effective system() spawn.