[PATCH] make "close" on viewport synchronous

[Qian Yun]

Currently, when doing "close" on a viewport, it simply
send a number (via socket) to the viewman process,
then return to the Spad REPL immediately, while the
view2D/view3D process is being closed asynchronously.

Thus causes the ugly hack "|waitForViewport|".

We can avoid this by making "close" synchronous:
now the "close" function wait for confirmation of
exit of view2D/view3D and resources cleanup.

- Qian

[Qian Yun]

Ping?

- Qian

[Waldek Hebisch]

On Wed, May 13, 2026 at 05:23:47PM +0800, Qian Yun wrote:
> Ping?

This one needs a bit more time to review.

--
Waldek Hebisch

[Waldek Hebisch]

Looks OK, go on. This in small irregularity in the protocol,
as "close" now needs double acknowledgement. AFAICS
we would get desired effect in 'get_graph_output' by
issuing any command to vieman, as viewman would respond
only after previous command has finished. But your
change probably is simpler.

Also, by making close synchronous you reduce concurrency
between FRICASsys and viewman, but that probably does not matter.

> diff --git a/src/algebra/view2D.spad b/src/algebra/view2D.spad
> index a9f96f6c..c305ec5f 100644
> --- a/src/algebra/view2D.spad
> +++ b/src/algebra/view2D.spad
> @@ -1020,6 +1020,8 @@ TwoDimensionalViewport () : Exports == Implementation where
> checkViewport viewport =>
> getI(VIEW) -- acknowledge
> viewport.key := 0$I
> + -- receive 100 from viewman, confirm view2D process is closed
> + getI(VIEW)
> void()
>
> coerce viewport ==
> diff --git a/src/algebra/view3D.spad b/src/algebra/view3D.spad
> index bf98b23e..06acc95f 100644
> --- a/src/algebra/view3D.spad
> +++ b/src/algebra/view3D.spad
> @@ -670,6 +670,8 @@ ThreeDimensionalViewport() : Exports == Implementation where
> checkViewport viewport =>
> getI(VIEW) -- acknowledge
> viewport.key := 0$I
> + -- receive 100 from viewman, confirm view3D process is closed
> + getI(VIEW)
> void()
>
> viewpoint (viewport : %) : V ==
> diff --git a/src/graph/viewman/viewman.c b/src/graph/viewman/viewman.c
> index f08cd6b5..44bafac0 100644
> --- a/src/graph/viewman/viewman.c
> +++ b/src/graph/viewman/viewman.c
> @@ -172,6 +172,8 @@ main (void)
> fprintf(stderr,"viewman: closing viewport\n");
> #endif
> closeChildViewport(slot);
> + /* notify Spad process that view2D/view3D process has been closed */
> + send_int(spadSock, 100);
> break;
>
> }; /* switch */
> diff --git a/src/hyper/htinp.c b/src/hyper/htinp.c
> index 7f1729c3..97ab8b45 100644
> --- a/src/hyper/htinp.c
> +++ b/src/hyper/htinp.c
> @@ -347,13 +347,6 @@ get_spad_output(FILE *pfile,char *command,int com_type)
> unescape_string(command);
> }
>
> -/*
> - * THEMOS says: There is a problem here in that we issue the (|close|) and
> - * then go on. If this is the last command, we will soon send a SIGTERM and
> - * the whole thing will collapse maybe BEFORE the writing out has finished.
> - * Fix: Call a Lisp function that checks (with \spadop{key} ps and grep) the
> - * health of the viewport. We do this after the (|close|).
> - */
> void
> get_graph_output(char *command,char *pagename,int com_type)
> {
> @@ -369,10 +362,10 @@ get_graph_output(char *command,char *pagename,int com_type)
> sprintf(buf, "(|processInteractive| '(|write| |%s| \"%s%d\" \"image\") NIL)", "%",
> pagename, example_number);
> send_lisp_command(buf);
> - send_lisp_command("(|setViewportProcess|)");
> - send_lisp_command("(|processInteractive| '(|close| (|%%| -3)) NIL)");
> - send_lisp_command("(|waitForViewport|)");
> + send_lisp_command("(|processInteractive| '(|close| (|%%| -2)) NIL)");
> + send_lisp_command("(|sockSendInt| |$MenuServer| 1)");
> get_int(spad_socket);
> + send_lisp_command("(|setIOindex| (- |$IOindex| 2))");
> }
> static void
> send_command(char *command,int com_type)
> diff --git a/src/interp/util.lisp b/src/interp/util.lisp
> index bb2ae3d6..b87e0552 100644
> --- a/src/interp/util.lisp
> +++ b/src/interp/util.lisp
> @@ -229,24 +229,3 @@ After this function is called the image is clean and can be saved.
>
> ;;; For evaluating categories we need to bind %.
> (defun |c_eval|(u) (let ((% '%)) (declare (special %)) (|eval| u)))
> -
> -;;; Accesed from HyperDoc
> -(defun |setViewportProcess| ()
> - (setq |$ViewportProcessToWatch|
> - (stringimage (CDR
> - (|processInteractive| '(|key| (|%%| -2)) NIL) ))))
> -
> -;;; Accesed from HyperDoc
> -(defun |waitForViewport| ()
> - (progn
> - (do ()
> - ((not (zerop (|run_shell_command|
> - (concat
> - "ps "
> - |$ViewportProcessToWatch|
> - " > /dev/null && sleep 0.1")))))
> - ())
> - (|sockSendInt| |$MenuServer| 1)
> - (|setIOindex| (- |$IOindex| 3))
> - )
> -)


--
Waldek Hebisch

[Waldek Hebisch]

On Sun, May 03, 2026 at 10:47:43AM +0800, Qian Yun wrote:

I just noticed that this patch causes trouble, do

draw(sin(x), x=0..2)

close the window with the plot, again do

draw(sin(x), x=0..2)

Now the second draw gives:

The viewport manager cannot find the requested graph and will quit and restart.

Repeating draw third time works OK. Fourth time again there is trouble.

After reverting the patch there is no such trouble.

[Qian Yun]

Yes, we can close the viewport from spad by "close",
or close it from X11 by clicking "x" button.

We need to distinguish the two close action.

See attachment for adding a slot variable for this case.

- Qian

[Waldek Hebisch]

On Sat, May 16, 2026 at 11:12:47AM +0800, Qian Yun wrote:
> Yes, we can close the viewport from spad by "close",
> or close it from X11 by clicking "x" button.

I did not look it that matters, but I was using 'quit' button
from graph control panel.

> We need to distinguish the two close action.
>
> See attachment for adding a slot variable for this case.
>
> - Qian
>
> On 5/16/26 10:17 AM, Waldek Hebisch wrote:
> >
> > I just noticed that this patch causes trouble, do
> >
> > draw(sin(x), x=0..2)
> >
> > close the window with the plot, again do
> >
> > draw(sin(x), x=0..2)
> >
> > Now the second draw gives:
> >
> > The viewport manager cannot find the requested graph and will quit and restart.
> >
> > Repeating draw third time works OK. Fourth time again there is trouble.
> >
> > After reverting the patch there is no such trouble.
> >

> diff --git a/src/graph/include/view2D.h b/src/graph/include/view2D.h
> index 7a849f40..9b90a406 100644
> --- a/src/graph/include/view2D.h
> +++ b/src/graph/include/view2D.h
> @@ -43,6 +43,7 @@ typedef struct _viewManager {
> char propertyName[14]; /* string pointing to the property name */
> Window viewWindow;
> struct _viewManager *nextViewport;
> + int closing;
> } viewManager;
>
> typedef struct _viewsWithThisGraph {
> diff --git a/src/graph/viewman/fun2D.c b/src/graph/viewman/fun2D.c
> index b61e24bb..a8d517e2 100644
> --- a/src/graph/viewman/fun2D.c
> +++ b/src/graph/viewman/fun2D.c
> @@ -160,6 +160,10 @@ funView2D(int viewCommand)
> code = write(viewport->viewOut,&i1,intSize);
> break;
>
> + case closeAll2D:
> + viewport->closing = 1;
> + break;
> +
> } /* switch */
> /*** get acknowledge from viewport */
> code = readViewport(viewport,&acknow,intSize);
> @@ -250,6 +254,7 @@ forkView2D(void)
> }
> viewport->viewType = view2DType;
> viewport->PID = childPID;
> + viewport->closing = 0;
>
> /* set up pipes to child process */
> close(pipe0[0]);
> diff --git a/src/graph/viewman/fun3D.c b/src/graph/viewman/fun3D.c
> index 64e59f2c..04fa79da 100644
> --- a/src/graph/viewman/fun3D.c
> +++ b/src/graph/viewman/fun3D.c
> @@ -249,6 +249,11 @@ funView3D(int viewCommand)
> i1 = get_int(spadSock);
> code = write(viewport->viewOut,&i1,intSize);
> break;
> +
> + case closeAll:
> + viewport->closing = 1;
> + break;
> +
> } /* switch */
> /*** get acknowledge from viewport */
>
> @@ -339,6 +344,7 @@ forkView3D(int typeOfViewport)
> }
> viewport->viewType = typeOfViewport;
> viewport->PID = childPID;
> + viewport->closing = 0;
>
> /* set up pipes to child process */
> close(pipe0[0]);
> diff --git a/src/graph/viewman/viewman.c b/src/graph/viewman/viewman.c
> index 44bafac0..c19e4eed 100644
> --- a/src/graph/viewman/viewman.c
> +++ b/src/graph/viewman/viewman.c
> @@ -171,9 +171,11 @@ main (void)
> #ifdef DEBUG

> fprintf(stderr,"viewman: closing viewport\n");
> #endif

> + int close_by_spad = slot->closing;
> closeChildViewport(slot);

> /* notify Spad process that view2D/view3D process has been closed */

> - send_int(spadSock, 100);
> + if (close_by_spad)

> + send_int(spadSock, 100);
> break;
>
> }; /* switch */

--
Waldek Hebisch

[Qian Yun]

On 5/16/26 11:31 AM, Waldek Hebisch wrote:
> On Sat, May 16, 2026 at 11:12:47AM +0800, Qian Yun wrote:
>> Yes, we can close the viewport from spad by "close",
>> or close it from X11 by clicking "x" button.
>
> I did not look it that matters, but I was using 'quit' button
> from graph control panel.
>

Clicking "x" button or "quit" button closes view2D process,
then it tells viewman process, in this case, it should not
send "100" to spadSock.


Also there is a use-after-free bug in this part of code,
if we close a viewport, pointer "slot" is freed.
The logic should be in the other if branch.

- Qian

[Waldek Hebisch]

Looks OK (I think both patches should go together). However, is
stepSlot used for anything? AFAICS we can remove it.

> diff --git a/src/graph/viewman/viewman.c b/src/graph/viewman/viewman.c
> index f08cd6b5..959d9788 100644
> --- a/src/graph/viewman/viewman.c
> +++ b/src/graph/viewman/viewman.c
> @@ -175,10 +175,11 @@ main (void)
> break;
>
> }; /* switch */
> -
> - }; /* if reading slot->viewIn */
> - stepSlot = slot;
> - slot = slot->nextViewport;
> + /* if reading slot->viewIn ends */
> + } else {
> + stepSlot = slot;
> + slot = slot->nextViewport;
> + }
> }; /* while */
>
> if (keepLooking) { /* if 1 => slots not read, read from spad */


--
Waldek Hebisch