Supports sending output to clients when running programs remotely #3253

tttoad · 2023-01-19T08:13:48Z

#3094
I saw that #3108 has not been updated for a long time, so I committed this PR.

tttoad · 2023-01-19T08:23:19Z

I am currently reading the code related to debug mode, and will support debug mode output write-back in this PR.

aarzilli

This needs to be supported on debugged processes as well, it would be weird to support this only when nodebug (I imagine this is just because it's a WIP). Also it needs a test. The tests are in service/dap/server_test.go.

service/dap/server.go

aarzilli · 2023-01-19T10:58:30Z

service/dap/server.go

+			wg := &sync.WaitGroup{}
+			readerFunc := func(reader io.Reader, category string, wg *sync.WaitGroup) {
+				// Display one line back after outputting one line
+				var scanner = bufio.NewScanner(reader)


I suspect it's a bad idea to add a layer of line buffering here, I think you should read directly from the reader into a buffer and send whatever you read down.

The output of the program may take a long time... I've thought about using a timed refresh and a fixed buffer size to achieve this, or maybe that's a better idea.

aarzilli · 2023-01-19T10:58:46Z

service/dap/server.go

+						s.send(&dap.OutputEvent{
+							Event: *newEvent("output"),
+							Body: dap.OutputEventBody{
+								Output:   fmt.Sprintln(out),


fmt.Sprintln doesn't do anything here.

scanner.Text() will drop n, so I need to add it back.

service/dap/server.go

tttoad · 2023-02-02T15:13:04Z

@aarzilli @derekparker PTAL.
I've tested it in the following environments.
(client: macOS arm64. server: ubuntu arm64 in docker)
(client: macOS arm64. server: macOS arm64)
(client: macOS arm64. server: windows amd64)
It can work.
I should be adding test cases next week when you think it's okay.

aarzilli · 2023-02-07T09:23:23Z

pkg/util/redirect.go

+)
+
+var (
+	redirectMap = &RedirectStroe{rs: sync.Map{}}


Sorry but passing around pipes by stuffing them in a global variable is not acceptable.

Only in windows system need to use the pipeline...
Using debugger.Config to pass the pipeline will change the definition of the native.Launch function.
Is this acceptable to you?

Yes. Also this will eventually need tests, both the nodebug path and the normal path. And Debugger.Restart needs to return an error in case pipe redirects are used (it would be even better if it worked correctly instead of erroring but that's hard and dap doesn't call Restart anyway).

aarzilli

Still doesn't have tests, also the (*Debugger).Restart function needs to return an error if it is called with pipe redirects.
Also it doesn't build.

aarzilli · 2023-02-11T16:13:59Z

pkg/util/redirect.go

@@ -0,0 +1,36 @@
+package util


This file shouldn't be used anymore and should be removed.

aarzilli · 2023-02-11T16:15:27Z

pkg/proc/redirect/redirect.go

+var ErrorNotImplemented = errors.New("not implemented")
+
+// Redirect
+type Redirect interface {


Make this a concrete type, put it in pkg/proc, not in its own package, make it describe a single redirect rather than three redirects in one. Do not repeat the type name in the name of its methods. For example, it should be (*Redirect).Path not (*Redirect).RedirectPath.

Restart is a word, it shouldn't be spelled ReStart.

make it describe a single redirect rather than three redirects in one

Still TBD.

Do you mean WriterFiles [3]*os.File -> StdinWriter *os.File, StdoutWriter *os.File, StderrWriter *os.File？

I think this should just be a string/io.Writer pair:

type OutputRedirect struct { Path string Writer io.Writer }

How about this?

type OutputRedirect struct { Path string File *os.File } Launch(.....,redirect [3]OutputRedirect)

I can't use io.Writer or io.WriteColser, because in gdbserial/rr.go it needs *os.File.
In service/service.go, the data will be read from this structure.

@aarzilli Need a reply.

What you suggested is ok.

aarzilli · 2023-02-11T16:18:59Z

service/dap/server.go

@@ -786,6 +807,13 @@ func (s *Session) handleRequest(request dap.Message) {
 }

 func (s *Session) send(message dap.Message) {
+	if event, ok := message.(*dap.OutputEvent); ok {


You can't hijack the send method to do other things, it should be done by the caller of this method, or maybe in a method that both waits for the readers to finish and then sends the terminated event.

service/dap/server.go

aarzilli · 2023-04-04T07:52:26Z

pkg/proc/gdbserial/rr.go

@@ -20,7 +20,7 @@ import (
 // program. Returns a run function which will actually record the program, a
 // stop function which will prematurely terminate the recording of the
 // program.
-func RecordAsync(cmd []string, wd string, quiet bool, redirects [3]string) (run func() (string, error), stop func() error, err error) {
+func RecordAsync(cmd []string, wd string, quiet bool, redirects [3]proc.OutputRedirect) (run func() (string, error), stop func() error, err error) {


I still think that this and other should be replaced by stdin string, stdout, stderr proc.OutputRedirect. They are not three of the same thing anymore, stdin is treated differently and there is no path for it becoming similar to the other two. Also stdin is definitely not an output.

aarzilli · 2023-04-04T08:02:25Z

pkg/proc/namedpipe_other.go

+	return oor.rd.Close()
+}
+
+func NamedPipe() (reader [2]io.ReadCloser, output [3]OutputRedirect, err error) {


It should be NamedPipe() (io.ReadClsoer, OutputRedirect, error). It's called NamedPipe not TwoNamedPipes

aarzilli · 2023-04-04T08:07:54Z

pkg/proc/proc_test.go

@@ -5926,6 +5927,120 @@ func TestStacktraceExtlinkMac(t *testing.T) {
 	})
 }

+func testGenRedirect(t *testing.T, fixture protest.Fixture, expectStdout string, expectStderr string, errChan chan error) (redirect [3]proc.OutputRedirect, cancelFunc func(), err error) {


IMO this functionality is adequately tested throught the dap layer and there is no need for this test here.

aarzilli · 2023-04-04T08:19:55Z

service/dap/server_test.go

+			stderr = bytes.NewBufferString("")
+		)
+
+	TerminnatedPoint:


terminatedPoint

aarzilli · 2023-04-04T08:22:28Z

pkg/proc/namedpipe_other.go

+	defer os.Remove(oor.p)
+
+	// try from "init" to "close".
+	if !atomic.CompareAndSwapInt32(&oor.status, 0, 2) {


This is awful, there has to be a better way. At minimum it should use a mutex instead.

I think it should be enough to start the readers only after the target process has started.

This can not work because rr.go (at minimum) would deadlock. However all we need to know is that the fifo has been opened for writing at least once, which we can do by unconditionally opening the fifo with os.O_WRONLY|syscall.O_NONBLOCK. This should work.

os.O_WRONLY|syscall.O_NONBLOCK can solve the blocking problem. But, until the pipe is opened in write mode, the read pipe will return null.

https://go.dev/play/p/hKYuUNNg868

I meant this:

func (oor *openOnRead) Close() error { defer os.Remove(oor.p) fh, _ := os.OpenFile(oor.p, os.O_WRONLY|syscall.O_NONBLOCK, 0) if fh != nil { fh.Close() } return oor.rd.Close() }

aarzilli

Just a couple of minor changes, otherwise it's OK.

Also, in (*Debugger).Restart after the canRestart check but before the call to detach add this:

if !resetArgs && (d.config.Stdout.File != nil || d.config.Stderr.File != nil) {
	return nil, ErrCanNotRestart
	
}

aarzilli · 2023-04-07T11:04:50Z

service/dap/server_test.go

+			stderr = bytes.NewBufferString("")
+		)
+
+	terminnatedPoint:


terminatedPoint

aarzilli · 2023-04-07T11:10:36Z

pkg/proc/gdbserial/rr.go

@@ -63,11 +63,13 @@ func RecordAsync(cmd []string, wd string, quiet bool, redirects [3]string) (run
 	return run, stop, nil
 }

-func openRedirects(redirects [3]string, quiet bool) (stdin, stdout, stderr *os.File, closefn func(), err error) {
-	toclose := []*os.File{}
+func openRedirects(stdinPath string, stdoutOR proc.OutputRedirect, stderrOR proc.OutputRedirect, quiet bool) (stdin, stdout, stderr *os.File, closefn func(), err error) {


just use stdin, stdout, stderr

stdin, stdout, stderr is in conflict with the returned parameter name.

aarzilli

LGTM. Thanks.

derekparker

Sorry for the review delay, just got back from some time off.

derekparker · 2023-04-24T20:43:46Z

pkg/proc/gdbserial/rr.go

-func openRedirects(redirects [3]string, quiet bool) (stdin, stdout, stderr *os.File, closefn func(), err error) {
-	toclose := []*os.File{}
+func openRedirects(stdinPath string, stdoutOR proc.OutputRedirect, stderrOR proc.OutputRedirect, quiet bool) (stdin, stdout, stderr *os.File, closefn func(), err error) {
+	var (


Please remove this var () block.

pkg/proc/namedpipe_other.go

derekparker · 2023-04-24T21:14:30Z

pkg/proc/namedpipe_other.go

+	return oor.rd.Close()
+}
+
+func NamedPipe() (reader io.ReadCloser, output OutputRedirect, err error) {


What's the purpose of this file and the Windows variant, why not simply use os.Pipe across the board?

Also, calling this a "named pipe" seems disingenuous as the caller cannot provide a name, so it's essentially the same as calling os.Pipe but with wrapped return values.

pkg/proc/gdbserial/gdbserver.go
The file path needs to be used in the gdbserver.LLDBLaunch(). I can't get the file path from os.Pipe.
It is called NamedPipe because it uses the Named Pipe mode. But this seems disingenuous in the namedpipe_other file. Maybe we can call it Redirector?

Ah, ok that makes sense. Yes, I like the name Redirector better, I think.

@derekparker PTAL.

pkg/proc/redirect.go

tttoad · 2023-05-10T01:19:15Z

@derekparker PTAL.

mcdoker18 · 2023-05-29T12:30:16Z

@tttoad Thanks for the implementing this feature!

@aarzilli @derekparker Can you please take a look again?

derekparker

LGTM

hyangah · 2023-10-05T23:54:08Z

service/dap/types.go

@@ -148,6 +148,9 @@ type LaunchConfig struct {
 	// reference to other environment variables is not supported.
 	Env map[string]*string `json:"env,omitempty"`

+	// The output mode specifies how to handle the program's output.
+	OutputMode string `json:"outputMode,omitempty"`


It needs to document what this is for and what values are acceptable. Please find how other string type fields were documented (e.g. Mode) and provide similar level of details.

hyangah · 2023-10-05T23:59:13Z

service/dap/server.go

+				n, err := reader.Read(out[:])
+				if err != nil {
+					if errors.Is(io.EOF, err) {
+						return


shouldn't we flush out the remaining bytes buffered in out (n>0) before returning if any?
From io.Reader document, it's possible n>0 and err != nil.

You're right. Thanks for your guidance.

hyangah · 2023-10-06T00:08:18Z

service/dap/server.go

+			return nil, err
+		}
+	} else {
+		cmd.Stdout, cmd.Stderr = os.Stdin, os.Stderr


should be
cmd.Stdout, cmd.Stderr = os.Stdout, os.Stderr

Fixes bugs introduced in v1.21.1 * Avoid dropping the last bytes from stderr/stdout when Read returns an error. (Read returns n>0). And skip sending Output event if Read returns n==0. * Fix the bug that drops all stdout in the existing noDebug mode. For go-delve#3253

Fixes bugs introduced in v1.21.1 * Avoid dropping the last bytes from stderr/stdout when Read returns an error. (Read returns n>0). And skip sending Output event if Read returns n==0. * Fix the bug that drops all stdout in the existing noDebug mode. For #3253

tttoad added 2 commits January 17, 2023 17:34

wip: Support sending output when remote debug

4a06698

wip: Support local output and remote output

6968a79

wip: fix stderr and stdout assignment error

105696d

aarzilli reviewed Jan 19, 2023

View reviewed changes

tttoad added 9 commits January 21, 2023 01:38

wip: optimize code

444eeb1

wip: Only if outputMode is "remote" is the redirected console output

3869956

wip: Redirected debugMode output(Not tested on windows)

8a43523

wip: support remote debugging output redirection of windows

4e47c69

wip: real-time write back output

8350405

wip: support for windows

9c96fbe

wip: fix windows remote debug not output

c4b8ce2

wip: fix truncated output redirection

a605ff9

Merge branch 'master' of github.com:tttoad/delve into feat-console

d56ac9f

wip: delete printfln

984e012

aarzilli reviewed Feb 7, 2023

View reviewed changes

wip: use debugger.Config to pass redirect(macOS)

63341e7

aarzilli reviewed Feb 11, 2023

View reviewed changes

tttoad added 11 commits February 12, 2023 01:19

wip: use debugger.Config to pass redirect(linux)

d5f5fb3

wip: Change redirect to a concrete type

4621f4c

wip: s.wg.wait before sending "terminated"

f433b23

wip: add proc/redirect test(darwin and linux)

ffad8a4

Merge branch 'master' of github.com:tttoad/delve into feat-console

a1467d6

Merge branch 'master' of github.com:tttoad/delve into feat-console

70216a5

wip: Fix test failure on windows

902235d

fix: undefined: proc.Redirects

9e08369

fix: compile failure

0b3c066

wip: Remove useless code

38958bc

fix: filename error

fc89882

refactor: namedpipe

702341d

aarzilli reviewed Apr 4, 2023

View reviewed changes

tttoad added 5 commits April 5, 2023 02:57

refactor: namedpipe, launch ...

5aeb3f8

fix: freebsd compile failure

10e0a66

fix: proc_darwin compile failure

1e3701b

style: remove useless code

4c8f86b

Merge branch 'master' of github.com:tttoad/delve into feat-console

c016bc1

aarzilli reviewed Apr 7, 2023

View reviewed changes

feat: add d.config.Stdxx check on debug.Restart

a4f0491

aarzilli approved these changes Apr 8, 2023

View reviewed changes

derekparker requested changes Apr 24, 2023

View reviewed changes

tttoad added 3 commits May 2, 2023 02:51

style: formatting and adding comments

0c2f3ad

style: formatting and adding comments

02d96ec

feat: add d.config.Stdxx check on debug.Restart

44fa283

tttoad added 3 commits May 12, 2023 23:33

style: namedpipe->redirector

768fad8

Merge branch 'feat-console' of github.com:tttoad/delve into feat-console

f27bfcf

style: namedPipe->redirector

7dd6f49

tttoad added 2 commits June 13, 2023 10:15

Merge branch 'master' into feat-console

7725eae

Merge branch 'master' of github.com:tttoad/delve into feat-console

38969ab

tttoad requested a review from derekparker June 29, 2023 14:51

derekparker approved these changes Jul 5, 2023

View reviewed changes

derekparker merged commit 53998cb into go-delve:master Jul 5, 2023
1 check passed

hyangah mentioned this pull request Oct 5, 2023

debug: noDebug mode & integratedTerminal mode tests are broken with delve v1.21.1 golang/vscode-go#2999

Closed

hyangah reviewed Oct 6, 2023

View reviewed changes

hyangah mentioned this pull request Oct 6, 2023

service/dap: fix bugs in stdout/stderr handling #3522

Merged

aarzilli mentioned this pull request Nov 16, 2023

Send DAP OutputEvents on stdout/stderr of debugged process when console is internalConsole #3094

Closed

Supports sending output to clients when running programs remotely #3253

Supports sending output to clients when running programs remotely #3253

Conversation

tttoad commented Jan 19, 2023

tttoad commented Jan 19, 2023

aarzilli left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tttoad commented Feb 2, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aarzilli left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aarzilli left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aarzilli left a comment

Choose a reason for hiding this comment

derekparker left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tttoad commented May 10, 2023

mcdoker18 commented May 29, 2023

derekparker left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aarzilli left a comment •

edited

Loading