Some versions of GLib under Linux continuously generate G_IO_IN-s without any data to read when using recursive channel watch sources, causing 100% CPU load. This patch detects such a situation, and automatically switches the affected source from channel watch to 50ms timeout.
A bit of explanation about the recursive sources. Let's say that a callback displays a modal dialog with an error message, or a prompt, something like that. A new message loop is generated for the dialog/prompt, but the source that initiated it is excluded, to avoid calling it's callback for a second time while the execution is still inside it. But, if the source is marked as "recursive", it will be invoked from the new message loop, provided that the source event conditions are met. It has to be implemented carefully, both in GLib and the application that uses it.
The only spawn() client that currently uses recursive sources is Scope, so the GLib bug manifests there. And a glib bug it should be, IMHO, because (a) no message loop is created which can activate recursion; (b) no recustion takes place (verified); and (c) even if a there was a recursion, the source event conditions are not met: these G_IO_IN-s come without any data to read, which is a bug by itself. You can view, comment on, or merge this pull request online at:
https://github.com/geany/geany/pull/1461
-- Commit Summary --
* Handle continuous G_IO_IN-s without any data
-- File Changes --
M src/spawn.c (68)
-- Patch Links --
https://github.com/geany/geany/pull/1461.patch https://github.com/geany/geany/pull/1461.diff
Closes https://github.com/geany/geany-plugins/issues/433
@zhekov pushed 1 commit.
673a714 Fix the maximum number of G_IO_IN-s without any data
kugel- commented on this pull request.
@@ -967,6 +999,23 @@ static gboolean spawn_read_cb(GIOChannel *channel, GIOCondition condition, gpoin
sc->cb.read(buffer, input_cond | failure_cond, sc->cb_data); } + /* Check for continuous activations with G_IO_IN | G_IO_PRI, without any + data to read and without errors. If detected, switch to timeout source. */ + else if (sc->empty_gio_ins < MAX_EMPTY_GIO_INS && status == G_IO_STATUS_AGAIN)
How do you know here that there was no data to read?
zhekov commented on this pull request.
@@ -967,6 +999,23 @@ static gboolean spawn_read_cb(GIOChannel *channel, GIOCondition condition, gpoin
sc->cb.read(buffer, input_cond | failure_cond, sc->cb_data); } + /* Check for continuous activations with G_IO_IN | G_IO_PRI, without any + data to read and without errors. If detected, switch to timeout source. */ + else if (sc->empty_gio_ins < MAX_EMPTY_GIO_INS && status == G_IO_STATUS_AGAIN)
"status" can be different from G_IO_STATUS_NORMAL only if we attempted to read. This means a (G_IO_IN | G_IO_PRI) condition, possibly combined with G_IO_FAILURE condition. We filter out the failures in the "if" clause, so a G_IO_STATUS_AGAIN means only (G_IO_IN | G_IO_PRI) condition at this point. But if there is no failure, spawn_read_cb() reads only once, for reasons explained in the source, and thus AGAIN is not a result of a subsequent read.
In summary, we received a G_IO_IN | G_IO_PRI, attempted to read, and immediately got G_IO_STATUS_AGAIN, which shoudn't happen...
kugel- commented on this pull request.
With this change scope does not freeze the Geany anymore. However, I was not able to enter commands in the gdb console (for which file descriptor this change is made for, right?).
@@ -871,13 +893,24 @@ static gboolean spawn_write_cb(GIOChannel *channel, GIOCondition condition, gpoi
}
+static gboolean spawn_read_cb(GIOChannel *channel, GIOCondition condition, gpointer data); + +static gboolean spawn_timeout_read_cb(gpointer data) +{ + SpawnChannelData *sc = (SpawnChannelData *) data; + + /* The solution for continuous empty G_IO_IN-s is to generate them outselves. :) */ + return spawn_read_cb(sc->channel, G_IO_IN, data);
Shouldn't we attempt to become a normal callback again, instead of staying on the timeout source indefinitely?
zhekov commented on this pull request.
@@ -871,13 +893,24 @@ static gboolean spawn_write_cb(GIOChannel *channel, GIOCondition condition, gpoi
}
+static gboolean spawn_read_cb(GIOChannel *channel, GIOCondition condition, gpointer data); + +static gboolean spawn_timeout_read_cb(gpointer data) +{ + SpawnChannelData *sc = (SpawnChannelData *) data; + + /* The solution for continuous empty G_IO_IN-s is to generate them outselves. :) */ + return spawn_read_cb(sc->channel, G_IO_IN, data);
Would you clarify "was not able to enter commands in the gdb console"? It works for me...
"for which file descriptor this change is made for, right?"
The change is for the spawn client reading from the child process output and error. It should not affect sending data to the child, such as sending a command from Scope to gdb.
"Shouldn't we attempt to become a normal callback again, instead of staying on the timeout source indefinitely?"
The empty gio IN-s for a channel seem to start as soon as data appears on that channel, and never end. Or, they may end after some unpredictable time (minutes or more), when the spawn client finally manages to read all data, and the 50ms timeouts will do that fast. But I have no reason to believe that the empty IN-s won't start again as soon as unread data appears on the channel.
The early versions of Scope (way before spawn) used 50ms timeouts as the simpler and most portable variant. They wasted ~10 seconds per hour on a single 2.2 GHz core IIRC. The new timeouts are a hack/emulation, so they may take more. Perhaps you can check that?
It would have been sooo much easier if glib had source suspend/resume...
Please don't merge ATM. It occurred to me that the timeout reads currently read only once per call = DEFAULT_IO_LENGTH each 50ms = 80KB/sec max, which is pretty low.
@zhekov pushed 1 commit.
1a19b39 Read stdout/stderr in a loop when using timeout callbacks
Fixed it, and moved the check whether the maximum empty inputs are reached to a macro.
Just a remainder: the throughput issue was fixed more than 2 months ago, and there are no other changes planned, so this PR is not a "work in progress". It would be nice to apply it at some point. If you have any questions, I'm ready to answer.
Looks reasonable, but I'll do some little testing. Do you have something more precise than "some versions of GLib" for reproducing?
There is https://github.com/geany/geany-plugins/issues/433 which I could easily reproduce. This is fixed by this patch.
I still think it should be attempted to become a normal callback again, it seems wasteful to keep a 50ms timer running indefinitely. Here is @zhekov reply to that:
The empty gio IN-s for a channel seem to start as soon as data appears on that channel, and never end. Or, they may end after some unpredictable time (minutes or more), when the spawn client finally manages to read all data, and the 50ms timeouts will do that fast. But I have no reason to believe that the empty IN-s won't start again as soon as unread data appears on the channel
That sounds to me as if we could switch to the timer only when there is data and go to the normal callback in idle periods.
@b4n @kugel-
The story so far:
Ubuntu 14.04 / glib 2.40 / Geany-1.30.1 - works. Ubuntu 16.04 / glib 2.48 / Geany-1.30.1 - hangs. Ubuntu 17.04 / glib 2.52 / Geany-1.30.1 - hangs.
I ran a "hanged" Geany for 1 hour, with some occasional traffic. It was extremely slow, of course, and never recovered by itself.
Then I left the time-sourced version idle for 1 hour on a "single" 3.0GHz virtual CPU. It wasted ~8.3 seconds.
Now, there is a chance that removing the timeout source and creating a new watch source will temporarily fix the input state. But replacing sources it not free either, and each time we switch from watch to timeout, we wait for 200 empty GIO_IN-s, which takes, say, 0.04 sec (YMMV). If we have 208 switches for an hour long debugging session - and each "Step" or automatic tooltip in Scope will likely generate a switch - the effect is zero.
Another thing I noted is that practically blocking the glib main loop has side effects. At one time, I activated File -> Open, then killed gdb, which freed the CPU, and the file open dialog was immediately displayed - but neither [Open] nor [Cancel] worked. So I'd rather avoid frequent empty G_IO_IN sequences.
Considering also the added complexity, I'm not going to write a switch-back-and-forth mechanism. This PR is a hack (gee), and I don't want to make it nastier than required.
TODO: test more glib versions: 2.53, 2.44, 2.??
zhekov on Apr 24: "...wasted ~10 seconds per hour on a single 2.2 GHz core IIRC."
Seems that I remembered well - this is close to the 8.3 seconds on a 3.0 GHz CPU...
@b4n
Debian 9 / glib 2.42.1 / Geany-1.30.1 - works. Debian 9 / glib >= 2.43.1 / Geany-1.30.1 - fails, including the latest 2.53.3.
So, the version which introduced the bug is *2.43*.
@b4n What do you think? I'm not 100% convinced that we shouldn't go back to normal mode of operation, however I think the current state is acceptable and the bug is rather annoying when it occurs (BUT I have noticed the bug in this one scenario with the scope debugger plugin).
@elextr @b4n @kugel-: I can confirm that this also fixed the problem on my system. Are there any plans to merge this in the next time?
Is there a chance that this could be added to the next release?
@LarsGit223 I think we're not yet determined if we want a workaround inside Geany at all, since it's apparently a bug in glib.
@b4n @codebrainz @elextr what do you think?
Would it take not too much time to sketch a call graph / involved threads that would be enough to create a minimal reproducer?
@kugel- sadly I think we have to have a work around in Geany, even if there is a fix in Glib, there are at least 10 versions in the wild with the problem by @b4n's post above, so either Geany becomes tolerant of those versions, or (IIUC) scope is unusable on systems with those versions (and more versions until its fixed).
So long as polling does not kick in on anything other than things associated with scope plugin I guess its an ok fix, even if it does not return to non-polling. As for adding to this release, unless @b4n is happy he has tested it enough I would wait, lets not have to have a 1.32.1 to fix the fix :)
Am 15.11.2017 um 15:40 schrieb elextr:
@kugel- https://github.com/kugel- sadly I think we have to have a work around in Geany, even if there is a fix in Glib, there are at least 10 versions in the wild with the problem by @b4n https://github.com/b4n's post above, so either Geany becomes tolerant of those versions, or (IIUC) scope is unusable on systems with those versions (and more versions until its fixed).
So long as polling does not kick in on anything other than things associated with scope plugin I guess its an ok fix, even if it does not return to non-polling. As for adding to this release, unless @b4n https://github.com/b4n is happy he has tested it enough I would wait, lets not have to have a 1.32.1 to fix the fix :)
The other side of the medal is that Geany itself is not affected, just a plugin that the author itself considers orphaned/unmaintaned, so I'm not sure a non-trivial change inside Geany is justified.
FWIW, I'm not against merging the change, I don't feel very strong. I'm rather indifferent since I don't use scope (doesn't support gtk3).
But if we accept it, then better for 1.32. This is sitting for a long time already and has been tested by at least two individuals already.
I wasn't aware that the Scope plugin was orphaned and unmaintained, I should have checked I guess.
In that case whats the rush to get this in 1.32? IMO it needs to be in git for a full release cycle to test it has no negative effects for those not using the Scope plugin and versions of Glib not exhibiting the problem. So if at all I would merge it just _after_ release.
1.32 is out. What are we gonna do here?
@b4n might have a say, he's assigned to this.
b4n commented on this pull request.
@@ -967,6 +1002,25 @@ static gboolean spawn_read_cb(GIOChannel *channel, GIOCondition condition, gpoin
sc->cb.read(buffer, input_cond | failure_cond, sc->cb_data); } + /* Check for continuous activations with G_IO_IN | G_IO_PRI, without any + data to read and without errors. If detected, switch to timeout source. */ + else if (SPAWN_CHANNEL_GIO_WATCH(sc) && status == G_IO_STATUS_AGAIN) + { + sc->empty_gio_ins++;
@zhekov shouldn't this be reset to 0 on successful read attempts (or decremented, I don't know) to avoid the occasional one to trigger the switch after a long while? Assuming it's not the occasional faulty one that trigger the big issue.
I'm not sure I totally get the implications of the issue, but it seems sad to switch to timeout if the faulty callbacks happens from time to time only -- let's say it happens every minute, it would switch to timeout source after 3h20 nonetheless.
zhekov commented on this pull request.
@@ -967,6 +1002,25 @@ static gboolean spawn_read_cb(GIOChannel *channel, GIOCondition condition, gpoin
sc->cb.read(buffer, input_cond | failure_cond, sc->cb_data); } + /* Check for continuous activations with G_IO_IN | G_IO_PRI, without any + data to read and without errors. If detected, switch to timeout source. */ + else if (SPAWN_CHANNEL_GIO_WATCH(sc) && status == G_IO_STATUS_AGAIN) + { + sc->empty_gio_ins++;
@b4n
Citing myself: "The empty gio IN-s for a channel seem to start as soon as data appears on that channel, and never end. Or, they may end after some unpredictable time (minutes or more) [...]"
AFAICT, the "successful" read attempts are only such because there incidently happens to be any data when the subsequent G_IN is delivered. If the counter is reset, there is a risk that small portions of data will be read at, say, each 100 G_IN-s, and Geany/Scope will work, but slow. I chose 200 as the limit because it does not cause a noticeable *one-time* slowdown before switching to timeouts even, on slow CPU-s.
I'm not against decrementing the counter, or even substracting some fixed ammount <= 10, if you consider that more safe. Just don't do it after the counter reaches maximum, it's also an indicator that input has switched to timeouts.
b4n commented on this pull request.
@@ -967,6 +1002,25 @@ static gboolean spawn_read_cb(GIOChannel *channel, GIOCondition condition, gpoin
sc->cb.read(buffer, input_cond | failure_cond, sc->cb_data); } + /* Check for continuous activations with G_IO_IN | G_IO_PRI, without any + data to read and without errors. If detected, switch to timeout source. */ + else if (SPAWN_CHANNEL_GIO_WATCH(sc) && status == G_IO_STATUS_AGAIN) + { + sc->empty_gio_ins++;
@zhekov OK. My reasoning was that if you don't switch right away, it suggests empty INs could happen in situations where switching to a timeout source is not a good idea or the thing to do; so that maybe they could happen rarely and not be a real problem hence not being worth switching.
But if you tell me the reason to wait for 200 empty INs before switching is e.g. not to bother switching for short-lived output, I get that and then it wouldn't we worth worrying about decrementing the value.
so what do I need to apply this fix? Is this in current master or do I need to clone master and apply the code snippet above to spawn.c ?
Am I right that this does not involve any changes to scope and I can keep the distro's geany-plugins ?
thx.
And a glib bug it should be, IMHO, because (a) no message loop is created which can activate recursion; (b) no recustion takes place (verified); and (c) even if a there was a recursion, the source event conditions are not met: these G_IO_IN-s come without any data to read, which is a bug by itself.
I would not expect a rapid response from glib but has this at least been submitted as a bug.
A geany work around would be nice, but as pointed out this should really be fixed at source and the last working version of glib2 is now rather old and really a feasible downgrade.
Am I right that this does not involve any changes to scope and I can keep the distro's geany-plugins ?
Yes, if your problems are related to this issue, then only geany itself needs to be changed and the plugins can stay unchanged.
A geany work around would be nice, but as pointed out this should really be fixed at source and the last working version of glib2 is now rather old and really a feasible downgrade.
However this is really a hack with unknown possible consequences for Geany and other plugins, just to support an unmaintained plugin that could go away at any time (and perhaps it should if it triggers this Glib bug).
@J-Dunn
so what do I need to apply this fix? Is this in current master or do I need to clone master and apply the code snippet above to spawn.c ?
Travis reports that all checks have passed, and there are no conflicts with master.
A geany work around would be nice, but as pointed out this should really be fixed at source [...]
I don't like the direction in which glib/gtk/gnome are going, and see no reason to help. If you are interested in my ranting, write to me personally, here is not the place.
@elextr
However this is really a hack with unknown possible consequences for Geany and other plugins
No other callers use recursive data sources, so they can't trigger the bug. Besides, a significant part of the spawn module is a hack for the broken-by-design glib spawn under Win~1.
just to support an unmaintained plugin that could go away at any time (and perhaps it should if it triggers this Glib bug)
Well, the README still says "report bugs to <my email>", so I try to fix them. Scope will go probably go away with gtk+2. After several years, judging from Debian statistics.
I have applied the patch and built geany and plugins from master in each case. Debugger no longer locks up bit I don't seem to get what I expect from the debugger. This may be my not knowing how to use it since it has never worked since I tried to install it. ```
int main () { #include <stdio.h> printf ("%s","hello worlds.\n\r"); return (0); } ```
I have build this with build command configured as follows:
gcc -g3 -Wall -c "%f"
the compiler window shows: ``` gcc -g3 -Wall -c "main" "main.c" (in directory: <local path to source>) Compilation finished successfully.
``` When I simply run it with the gear wheel icon, I get a vte terminal with the output msg. Fine.
When I set break point in the return in line 4 and try to run it with Debug|Run/Continue the status bar appears show "Busy" but I do not see anything in Stack, Memory or Program Terminal tabs.
None of the debug functions like step are enabled, only 'terminate' is possible.
At This point I was expecting to be able to step the prog in the debugger and to already see the 'hello worlds' in a terminal somewhere. Also I was expecting some indication in the editor that execution was halted on line 4 or main.c ; this does not seem to be the state of play.
As far as I can tell from limited help description, this should be what is happening at this stage.
Am I missing something or are there still problems with debugging after the spawn lockup is fixed?
If you specify option ```-c``` then your program is only compiled but not linked. I guess you need to ommit the ```-c``` option. The debugger needs an executable.
You need to specify the executable file under ```Debug/Setup Program```.
Thanks, I'd already removed -c but must have copied that info first. ``` gcc -g3 -Wall -o "main" "main.c" (in directory:<path>) Compilation finished successfully.
```
I have a new file 'main' with the a current timestamp.
Result is the same as before.
I have used scope before successfully and just re-tested with your example code and it works for me. This is what I did: - copied spawn.c bugfix code and re-compiled geany - started geany and activated scope plugin - copied your example code to hello.c - ran your compiler command - in Debug/Setup Program I set: - Executable: home/lars/test/hello - Working dir: home/lars/test - all other settings are unchanged - set a breakpoint at the ```printf()``` line - clicked on "Run/continue" - progam starts to run, a yellow arrow appears on the breakpoint line - I clicked on "step over" and program execution continues...
Seems fine for me. I am using Ubuntu 16.04.
thanks for testing Lars. That is the same as what I'm doing ( except for main instead of hello ). I moved the breakpoint to the printf to be exactly the same. Do you see an output terminal with the text output?
I get no arrow , no debugging options other than terminate. I does not seem to be running or connecting to gdb.
Should probably move the discussion to the mailing list as it's rather off-topic for this Issue, plus more people watch the mailing list.
The code snippet given by Kugel needs some context because line 900 now is a different part of the code ! ``` static void spawn_destroy_cb(gpointer data) { SpawnChannelData *sc = (SpawnChannelData *) data; ```
it seems that this now needs to be inserted at line 956.
Hi, I am getting nowhere fixing this bug. It no longer locks up the screen but it does not provide gdb connection.
Could someone confirm I have inserted the right patch at the right line in src/spawn.c
``` gsize n = line_buffer->len;
while ((status = g_io_channel_read_chars(channel, line_buffer->str + n, DEFAULT_IO_LENGTH, &chars_read, NULL)) == G_IO_STATUS_NORMAL) { SpawnChannelData *sc = (SpawnChannelData *) data;
/* The solution for continuous empty G_IO_IN-s is to generate them outselves. :) */ return spawn_read_cb(sc->channel, G_IO_IN, data); ```
I've been through this a dozen times today and it's still not producing anything in the debugger. I have run the sample prog from command line gdb and all is fine. But I do not get any output from Scope. Where should I see this ? Does it produce a VTE terminal like geany gear icon does or is it in one of the tabs in geany?
The only two debug icons that are not greyed out are 'toggle breakpoint' and 'terminate'. Both produce the expected results.
There must be a problem somewhere but it does not seem to be being reported. in help | debug messages; ```
21:44:50: Geany INFO : Added filetype Clojure (67). 21:44:50: Geany INFO : Added filetype Scala (68). 21:44:50: Geany INFO : Loaded libvte from libvte.so 21:44:50: GLib-GObject WARNING : gsignal.c:2523: signal 'project-before-save' is invalid for instance '0x1538a20' of type 'GeanyObject' 21:44:50: Geany INFO : Loaded: /usr/local/lib/geany/scope.so (Scope Debugger) 21:44:50: Geany INFO : unknown : None (UTF-8) ```
I run geany from command line but the only output there is the gsignal warning.
``` gdb --version GNU gdb (GDB) Fedora 8.0.1-33.fc26
I love geany as a code editor but I'd like to use its IDE capabilities. I'm desperate to dump the lumbering eclipse java dinosaur which takes about 5s just to step a single line of code :x
```
OK. I did not realise that this thread itself contained the modified code. I saved the raw spawn.c and now it works. Phew. https://raw.githubusercontent.com/zhekov/geany/1a19b397216210b9b5f19592da7bb...
Thanks to zhekov for providing the fix.
It would be great to get this merged for 1.33 to avoid mucking about with the build system disabling perfectly functioning plugins and leaving users with the choice of not having the good debugger plugin or having a buggy one. It's been ready for almost a year, has been tested by multiple people, and was meant to be included in the last release. IMO, this makes it an ideal candidate for merging even as we approach the next release.
@b4n any thoughts? what's left to do to make this mergable?
Some users have tested it fixes scope, not that it doesn't have any side effects. It should not be merged just before release when it will not get tested by a wider range of users first. To foist this on all Geany users for the sake of a one work change in the G-P build system is a strange tradeoff.
Scope doesn't even supper gtk3 which many distros seem to have switched to, so it's not actually working that fine.
I used the same arguments prior to the last release but I'm not sure anymore if scope is worth it
I was very pleased to get remote debugging working cross-debugger . Don't tell me it's dead before I even get to use it.
@J-Dunn nobodys killing anything, [DON'T PANIC] :)
Just that in the past there have been side effects from last minute additions that weren't tested properly. So some of us don't want to take the risk, and some do, so its up to @b4n to decide.
And if the patch is not included, I have suggested that scope should not be distributed by default with this release so as not to hang users Geany unexpectedly.
But in the long term scope needs a maintainer, and when someone takes that job they may decide to modify how it works so it doesn't trigger the bug, instead of this patch. The maintainer may also port it to GTK3.
b4n commented on this pull request.
@@ -967,6 +1002,25 @@ static gboolean spawn_read_cb(GIOChannel *channel, GIOCondition condition, gpoin
sc->cb.read(buffer, input_cond | failure_cond, sc->cb_data); } + /* Check for continuous activations with G_IO_IN | G_IO_PRI, without any + data to read and without errors. If detected, switch to timeout source. */ + else if (SPAWN_CHANNEL_GIO_WATCH(sc) && status == G_IO_STATUS_AGAIN) + { + sc->empty_gio_ins++;
BTW, does the condition include `G_IO_IN | G_IO_PRI` at all? Here you use `! (G_IO_ERR | G_IO_HUP | G_IO_NVAL)`, which seems reasonable at first, but if it's a bug in GLib maybe the condition is just empty or even `G_IO_OUT`? Anyway, that doesn't change much, just curiosity on my part.
I didn't test this myself, and am still unclear with the root cause of the issue, but I trust @zhekov's explanations and the code look OK to me.
I however tend to agree with @elextr and @kugel-: merge it, but right after the release so we get testing. It's tricky as we always say this and then forget until next cycle, but maybe we can do better this time… And yeah, more and more distros seem to switch to GTK3 so Scope won't work there now.
b4n commented on this pull request.
@@ -871,13 +893,24 @@ static gboolean spawn_write_cb(GIOChannel *channel, GIOCondition condition, gpoi
}
+static gboolean spawn_read_cb(GIOChannel *channel, GIOCondition condition, gpointer data); + +static gboolean spawn_timeout_read_cb(gpointer data) +{ + SpawnChannelData *sc = (SpawnChannelData *) data; + + /* The solution for continuous empty G_IO_IN-s is to generate them outselves. :) */ + return spawn_read_cb(sc->channel, G_IO_IN, data);
It would have been sooo much easier if glib had source suspend/resume...
can't one simply ref the source, remove it from the context, add it back and unref it as a mean to pause it?
Quick (and not yet thorough) testing suggest it doesn't affect regular Geany features (FiF, build, custom commands) -- just as it's supposed to.
Once again: no other callers use recursive data sources, so they can't trigger the bug. If the non-recursive sources were buggy, believe me, you would have known.
Yet I do understand your concerns, especially about the plugin being unmaintained and limited to gtk+2. If you don't want to merge the PR, *please* disable Scope for the Linux release. Distributing software which is known not to work, labeled with my name, is not very pleasant.
I don't understand the gtk+2 problem either. Don't distros ensure multiple versions of such base libraries can be installed concurrently?
@J-Dunn a system may have both GTK2 and GTK3 apps, but an individual app like Geany may only be GTK2 or GTK3, not both. But some plugins only support GTK2. So the version of Geany (gtk2 or gtk3) that the distros decide to distribute also determines which plugins will work.
Isn't all this taken care of by the automake process?
If a distro , or anyone else, tries to build with gtk3 then attempting to build certain plugins should fail at the configure stage. The builder will then have to choose whether to stick with gtk+2 and have everything working or be a gtk3 zealot and deprive their user base of the plugins which can not be complied with gtk3.
The choice to build Geany with GTK2 or 3 is made at configure time `--enable-gtk3`, and then when plugins are built you specify which Geany to use and thats whats determines if the plugins are built gtk2 or 3.
I should add, if you build Geany and plugins yourself you can choose what you want to do.
Similarly distros can choose what they decide to provide. Some are actively trying to move away from GTK2 probably to move toward not installing more than two GTK versions by default when GTK4 is released later this year. Also they may be anticipating that support for GTK2 will drop off when GTK4 is released. But its their choice, Geany itself works with both GTK2 and GTK3 and many of the Geany devs now use the GTK3 version.
Ping. Release 1.33 is out. IMHO if the patch in geany shall be merged to master it should be done a.s.a.p. to increase the testing time.
@b4n, ping, you have self assigned.
FGS lets make a decision! Flip a coin, rand(), anything!!! :grin:
Either scrap this and scope, or apply this NOW so it can get tested for side effects outside scope by @eht16 and @codebrainz and other windows users and @techee for OSX.
But be ready to revert if any problems occur, or if nobody tests it.
Despite repeated statements by zhekov that nothing else uses this mechanism, so nothing else CAN be affected, this idea still seems to be being pushed. If no one has a contrary opinion or can point out something else that COULD be affected , the whole question seems mute.
Sure, testing is always good policy and reassuring but where is the problem here?
@J-Dunn the problem this change is intended to mitigate a bug in GIO. It is not a fix, it just limits the damage by limiting throughput in the subprocess pipes and in doing that slows operation and increases CPU usage. It is a change in Geany, not in Scope so it is not limited to subprocess IO in Scope and would apply if triggered by any of Geany's many subprocess operations: builds, find in files, context commands etc.
But the bug appears to only exist in some versions of GIO so it clearly is caused by changes made in the GIO code.
@zhekov is likely correct in stating that the mitigation is not triggered by anything other than the particular usage in the Scope plugin for the version of GIO he examined, but that is relying on the people who caused the bug in the first place to not make any other changes to GIO that affect that current incorrect behaviour so as to not affect other users.
Other than examining the code of many versions of GIO, testing is the only way of telling if anything else is affected. Especially on Windows and OSX where the subprocess code is likely different to that on Linux. I expect this has not been tested on those systems (and hence my ping of known users there, apologies to those I missed). Also since the Scope plugin does not currently run on GTK3 and so nobody with GTK3 has any reason to apply the patch I bet it hasn't been tested their either, sure its should not matter, but its a bug, who knows.
So in relation to this hack testing is not just good policy, its essential to provide a feeling of comfort that the hack won't be triggered outside the intended situation. Note its not definitive, just an indication that it seems to be ok.
Maybe the solution is for a copy of the spawn code with the patch to be made in scope so it only applies to Scope.
Maybe the solution is for a copy of the spawn code with the patch to be made in scope so it only applies to Scope.
Ah, that does sound like a tidier and safer solution.
Maybe the solution is for a copy of the spawn code with the patch to be made in scope so it only applies to Scope.
Good idea. Not beautiful because of the code duplication but maybe the only safe way to apply the workaround with a 100% guarantee for having no side effects to non-scope users. I already created a PR.
Not beautiful because of the code duplication but maybe the only safe way to apply the workaround with a 100% guarantee for having no side effects to non-scope users.
The other way being to merge it so more people test it.
Merged #1461.
I don't think copying the spawn module is a good idea, it was actually moved from the plugin because subprocess communication is a tricky (esp. when you want it to be portable, but even otherwise there's a gazillion ways to shoot yourself in the food without even realizing it) and important piece of code. I just merged it as it's early in the release cycle and we'll get some testing (hopefully :crossed_fingers:).
I'll try and test on Window 7 and 10 this weekend once the nightlies update.
github-comments@lists.geany.org