gnuplot-beta

Fixes to X11_waitforinput()

[Dima K. <gn...@di...>]

Hi.

I've been working on fixing broken "pause mouse close" behavior on the
x11 terminal while x-forwarding. This is

  https://sourceforge.net/p/gnuplot/bugs/2208/

In the process I discovered that I already encountered weirdness in this
area previously:

  https://sourceforge.net/p/gnuplot/mailman/message/36440354/

I just fixed all the issues, I think. These are in 2 patches on this
branch:

  https://github.com/dkogan/gnuplot/commits/fixed-x11-waitforinput

The first patch improves diagnostic printing in this area. Instead of
printing enums as NUMBERS, it prints the enum NAMES. This is infinitely
more useful.

The second patch contains the actual fixes. It looks more invasive than
it is. 99% of the modifications are in X11_waitforinput(). And much of
it is indentation.

There were several issues, all of them caused by hoaky input
multiplexing. I'm attaching two very simple gnuplot scripts that tickled
the bug. To observe the problems you need to slow down the X connection
by ssh-ing to any server, and trying to plot through x-forwarding.
Before any of the fixes this would happen:

    $ gnuplot < mild.pause.mouse.close.broken.gp

    gnuplot> ause mouse close
             ^
             line 0: invalid command

This was caused by this block of code:

    if (IPC_LOCK) {
#ifdef HAVE_USLEEP
	usleep(100);
#endif
        if (repeat_count++ < 10000)
	    goto AGAIN;
    }

The slower link means that 10000 is too low. Bumping it up fixes the
problem for this data file. But if you then run the other test file, you
still see issues:

    $ gnuplot < hard.pause.mouse.close.broken.gp

    gnuplot> splse mouse close
             ^
             line 0: invalid command

This is an entirely different bug. I chased it down a few days ago, so I
don't remember the exact details. The gist of the issue was that we
would read some commands from ipc_back_fd then we would read some stuff
from stdin, and then we would go back, and read the rest of the stuff
from ipc_back_fd. This non-atomicity would break stuff. The fix was to
read ALL the available ipc_back_fd data when possible, BEFORE reading
anything from stdin.

The existing code was hoaky and mysterious. There were weird sleeps all
over the place, and lots of state. The patch here refactors
X11_waitforinput() to get rid of the sleeps and unneeded loops. The
IPC_LOCK is gone, and is now another possible option argument to
X11_waitforinput(). And there're better comments, I think.

It works for me so far, but this is an error-prone area. Can somebody
take a look? The new approach is much better, I think, but more testing
would be good.

Thanks.

Re: Fixes to X11_waitforinput()

[Ethan M. <eam...@gm...>]

On Saturday, 14 March 2020 23:12:24 PDT Dima Kogan wrote:
> Hi.
> 
> I've been working on fixing broken "pause mouse close" behavior on the
> x11 terminal while x-forwarding. This is
> 
>   https://sourceforge.net/p/gnuplot/bugs/2208/
> 
> In the process I discovered that I already encountered weirdness in this
> area previously:
> 
>   https://sourceforge.net/p/gnuplot/mailman/message/36440354/
> 
> I just fixed all the issues, I think. These are in 2 patches on this
> branch:
> 
>   https://github.com/dkogan/gnuplot/commits/fixed-x11-waitforinput

Can you send me an idiot's guide to how to extract those patches from
the github site?   I can view them as html pages, but cannot figure out
how to download in a form I can feed to `patch`. 

Alternatively can you tell me, still a naive git user, how to fetch or
cherry-pick these commits from your git instance?

	thanks,

		Ethan


> The first patch improves diagnostic printing in this area. Instead of
> printing enums as NUMBERS, it prints the enum NAMES. This is infinitely
> more useful.
> 
> The second patch contains the actual fixes. It looks more invasive than
> it is. 99% of the modifications are in X11_waitforinput(). And much of
> it is indentation.
> 
> There were several issues, all of them caused by hoaky input
> multiplexing. I'm attaching two very simple gnuplot scripts that tickled
> the bug. To observe the problems you need to slow down the X connection
> by ssh-ing to any server, and trying to plot through x-forwarding.
> Before any of the fixes this would happen:
> 
>     $ gnuplot < mild.pause.mouse.close.broken.gp
> 
>     gnuplot> ause mouse close
>              ^
>              line 0: invalid command
> 
> This was caused by this block of code:
> 
>     if (IPC_LOCK) {
> #ifdef HAVE_USLEEP
> 	usleep(100);
> #endif
>         if (repeat_count++ < 10000)
> 	    goto AGAIN;
>     }
> 
> The slower link means that 10000 is too low. Bumping it up fixes the
> problem for this data file. But if you then run the other test file, you
> still see issues:
> 
>     $ gnuplot < hard.pause.mouse.close.broken.gp
> 
>     gnuplot> splse mouse close
>              ^
>              line 0: invalid command
> 
> This is an entirely different bug. I chased it down a few days ago, so I
> don't remember the exact details. The gist of the issue was that we
> would read some commands from ipc_back_fd then we would read some stuff
> from stdin, and then we would go back, and read the rest of the stuff
> from ipc_back_fd. This non-atomicity would break stuff. The fix was to
> read ALL the available ipc_back_fd data when possible, BEFORE reading
> anything from stdin.
> 
> The existing code was hoaky and mysterious. There were weird sleeps all
> over the place, and lots of state. The patch here refactors
> X11_waitforinput() to get rid of the sleeps and unneeded loops. The
> IPC_LOCK is gone, and is now another possible option argument to
> X11_waitforinput(). And there're better comments, I think.
> 
> It works for me so far, but this is an error-prone area. Can somebody
> take a look? The new approach is much better, I think, but more testing
> would be good.
> 
> Thanks.
> 
> 

Re: Fixes to X11_waitforinput()

[Dima K. <gn...@di...>]

Ethan Merritt <eam...@gm...> writes:

> Can you send me an idiot's guide to how to extract those patches from
> the github site?   I can view them as html pages, but cannot figure out
> how to download in a form I can feed to `patch`.
>
> Alternatively can you tell me, still a naive git user, how to fetch or
> cherry-pick these commits from your git instance?

Sure

  cd ethans_gnuplot_directory
  git remote add dima gi...@gi...:dkogan/gnuplot.git
  git fetch dima

The patches are now reference-able as

  dima/fixed-x11-waitforinput^

and

  dima/fixed-x11-waitforinput

You can 'git cherry-pick' these, or whatever else you want to do.

Re: Fixes to X11_waitforinput()

[Ethan A M. <me...@uw...>]

On Saturday, 14 March 2020 23:12:24 PDT Dima Kogan wrote:
> Hi.
> 
> I've been working on fixing broken "pause mouse close" behavior on the
> x11 terminal while x-forwarding. This is
> 
>   https://sourceforge.net/p/gnuplot/bugs/2208/
> 
> In the process I discovered that I already encountered weirdness in this
> area previously:
> 
>   https://sourceforge.net/p/gnuplot/mailman/message/36440354/
> 
> I just fixed all the issues, I think. These are in 2 patches on this
> branch:
> 
>   https://github.com/dkogan/gnuplot/commits/fixed-x11-waitforinput
> 
> The first patch improves diagnostic printing in this area. Instead of
> printing enums as NUMBERS, it prints the enum NAMES. This is infinitely
> more useful.
> 
> The second patch contains the actual fixes. It looks more invasive than
> it is. 99% of the modifications are in X11_waitforinput(). And much of
> it is indentation.
> 
> There were several issues, all of them caused by hoaky input
> multiplexing. I'm attaching two very simple gnuplot scripts that tickled
> the bug. To observe the problems you need to slow down the X connection
> by ssh-ing to any server, and trying to plot through x-forwarding.
> Before any of the fixes this would happen:
> 
>     $ gnuplot < mild.pause.mouse.close.broken.gp
> 
>     gnuplot> ause mouse close
>              ^
>              line 0: invalid command
> 

> [...snip...]

> It works for me so far, but this is an error-prone area. Can somebody
> take a look? The new approach is much better, I think, but more testing
> would be good.

It has been a long time since I looked at the x11 input multiplexing code
but I remember that it was a mess and many observed problems were
dealt with by slapping a special case in rather than rethinking the design.
So yes, a re-write is a good thing.

Your revised code works well in all the tests that I ran, but I am sure that
there are people with use-cases I didn't test.  If you are reading this and
find a problem with the new code, let us know.

Anyhow, I've imported your fixes into the main code repository.
Keep up the good work. 
	
	Ethan

Re: Fixes to X11_waitforinput()

[Dima K. <gn...@di...>]

Ethan A Merritt <me...@uw...> writes:

> Your revised code works well in all the tests that I ran, but I am
> sure that there are people with use-cases I didn't test. If you are
> reading this and find a problem with the new code, let us know.

OK, great to hear. I'm going to use these patches in my day-to-day to
see if I hit any issues. Will post here if I do.

Thanks.