-
Notifications
You must be signed in to change notification settings - Fork 2.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Supports sending output to clients when running programs remotely #3253
Changes from 14 commits
4a06698
6968a79
105696d
444eeb1
3869956
8a43523
4e47c69
8350405
9c96fbe
c4b8ce2
a605ff9
d56ac9f
984e012
63341e7
d5f5fb3
4621f4c
f433b23
ffad8a4
a1467d6
70216a5
902235d
9e08369
0b3c066
38958bc
fc89882
e6a0c78
359ee2c
ad0dec1
73c7076
61092e2
bdd13c9
f43b3d3
28cea47
c0b0d07
4617c1f
123de13
980d430
72c6718
f83ec15
31dba49
8ebdb9d
7d441c2
dfa96ba
ca76f9d
36d0e92
6e33d4a
702341d
5aeb3f8
10e0a66
1e3701b
4c8f86b
c016bc1
a4f0491
0c2f3ad
02d96ec
44fa283
768fad8
f27bfcf
7dd6f49
7725eae
38969ab
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,20 @@ | ||
package redirect | ||
|
||
import ( | ||
"errors" | ||
"os" | ||
) | ||
|
||
var ErrorNotImplemented = errors.New("not implemented") | ||
|
||
// Redirect | ||
type Redirect interface { | ||
// RedirectPath | ||
RedirectPath() (redirects [3]string, err error) | ||
// RedirectFile | ||
RedirectReaderFile() (readerFile [3]*os.File, err error) | ||
// RedirectWriterFile | ||
RedirectWriterFile() (writerFile [3]*os.File, err error) | ||
// ReStart | ||
ReStart() error | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,36 @@ | ||
package util | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This file shouldn't be used anymore and should be removed. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. OK. |
||
|
||
import ( | ||
"os" | ||
"sync" | ||
) | ||
|
||
var ( | ||
redirectMap = &RedirectStroe{rs: sync.Map{}} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sorry but passing around pipes by stuffing them in a global variable is not acceptable. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Only in windows system need to use the pipeline... There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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). |
||
) | ||
|
||
type RedirectStroe struct { | ||
rs sync.Map | ||
} | ||
|
||
func GetRedirectStrore() *RedirectStroe { | ||
return redirectMap | ||
} | ||
|
||
func (r *RedirectStroe) Store(key string, val *Pipe) { | ||
r.rs.Store(key, val) | ||
} | ||
|
||
func (r *RedirectStroe) Load(key string) (*Pipe, bool) { | ||
val, ok := r.rs.Load(key) | ||
if ok { | ||
return val.(*Pipe), true | ||
} | ||
|
||
return nil, false | ||
} | ||
|
||
type Pipe struct { | ||
Reader *os.File | ||
Writer *os.File | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -159,8 +159,29 @@ type Session struct { | |
// changeStateMu must be held for a request to protect itself from another goroutine | ||
// changing the state of the running process at the same time. | ||
changeStateMu sync.Mutex | ||
|
||
// outputMode specifies how to print the program's output. | ||
outputMode outputMode | ||
|
||
// stdoutReader the programs's stdout. | ||
stdoutReader io.ReadCloser | ||
|
||
// stderrReader the program's stderr. | ||
stderrReader io.ReadCloser | ||
|
||
// wg the WaitGroup that needs to wait before sending a terminated event. | ||
wg sync.WaitGroup | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This should have a more descriptive name instead of just wg. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. done. |
||
} | ||
|
||
type outputMode int8 | ||
|
||
const ( | ||
// outputToStd os.Stdin and os.Stdout | ||
outputToStd outputMode = 1 << iota | ||
// outputToDAP Sending program output to the client. | ||
outputToDAP | ||
) | ||
|
||
// Config is all the information needed to start the debugger, handle | ||
// DAP connection traffic and signal to the server when it is time to stop. | ||
type Config struct { | ||
|
@@ -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 { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
if event.GetEvent().Event == "terminated" { | ||
// wait for all tasks(output redirects) to finish | ||
s.wg.Wait() | ||
} | ||
} | ||
|
||
jsonmsg, _ := json.Marshal(message) | ||
s.config.log.Debug("[-> to client]", string(jsonmsg)) | ||
// TODO(polina): consider using a channel for all the sends and to have a dedicated | ||
|
@@ -974,7 +1002,7 @@ func (s *Session) onLaunchRequest(request *dap.LaunchRequest) { | |
|
||
var cmd string | ||
var out []byte | ||
var err error | ||
|
||
switch args.Mode { | ||
case "debug": | ||
cmd, out, err = gobuild.GoBuildCombinedOutput(args.Output, []string{args.Program}, args.BuildFlags) | ||
|
@@ -1025,9 +1053,61 @@ func (s *Session) onLaunchRequest(request *dap.LaunchRequest) { | |
argsToLog.Cwd, _ = filepath.Abs(args.Cwd) | ||
s.config.log.Debugf("launching binary '%s' with config: %s", debugbinary, prettyPrint(argsToLog)) | ||
|
||
var redirected = false | ||
s.outputMode |= outputToStd | ||
switch args.OutputMode { | ||
aarzilli marked this conversation as resolved.
Show resolved
Hide resolved
|
||
case "remote": | ||
s.outputMode |= outputToDAP | ||
redirected = true | ||
case "only-remote": | ||
s.outputMode = outputToDAP | ||
redirected = true | ||
case "local", "": | ||
// noting | ||
default: | ||
s.sendShowUserErrorResponse(request.Request, FailedToLaunch, "Failed to launch", | ||
fmt.Sprintf("invalid debug configuration - unsupported 'outputMode' attribute %q", args.OutputMode)) | ||
return | ||
} | ||
|
||
readerFunc := func(reader io.Reader, category string) { | ||
var stdWriter io.Writer | ||
if category == "stdout" { | ||
stdWriter = os.Stdout | ||
} else { | ||
stdWriter = os.Stderr | ||
} | ||
|
||
var out [1024]byte | ||
for { | ||
n, err := reader.Read(out[:]) | ||
if err != nil { | ||
if errors.Is(io.EOF, err) { | ||
return | ||
} | ||
s.config.log.Errorf("failed read by %s - %v ", category, err) | ||
return | ||
} | ||
outs := string(out[:n]) | ||
|
||
if s.outputMode&outputToStd != 0 { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This can never happen now. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. yes.I will remove the part related to There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. done. |
||
fmt.Fprintf(stdWriter, outs) | ||
} | ||
|
||
if s.outputMode&outputToDAP != 0 { | ||
s.send(&dap.OutputEvent{ | ||
Event: *newEvent("output"), | ||
Body: dap.OutputEventBody{ | ||
Output: outs, | ||
Category: category, | ||
}}) | ||
} | ||
} | ||
} | ||
|
||
if args.NoDebug { | ||
s.mu.Lock() | ||
cmd, err := s.newNoDebugProcess(debugbinary, args.Args, s.config.Debugger.WorkingDir) | ||
cmd, err := s.newNoDebugProcess(debugbinary, args.Args, s.config.Debugger.WorkingDir, redirected) | ||
s.mu.Unlock() | ||
if err != nil { | ||
s.sendShowUserErrorResponse(request.Request, FailedToLaunch, "Failed to launch", err.Error()) | ||
|
@@ -1039,16 +1119,65 @@ func (s *Session) onLaunchRequest(request *dap.LaunchRequest) { | |
|
||
// Start the program on a different goroutine, so we can listen for disconnect request. | ||
go func() { | ||
if redirected { | ||
s.wg.Add(1) | ||
go func() { | ||
defer s.wg.Done() | ||
readerFunc(s.stdoutReader, "stdout") | ||
}() | ||
|
||
s.wg.Add(1) | ||
go func() { | ||
defer s.wg.Done() | ||
readerFunc(s.stderrReader, "stderr") | ||
}() | ||
// Wait for the input and output to be read | ||
} | ||
if err := cmd.Wait(); err != nil { | ||
s.config.log.Debugf("program exited with error: %v", err) | ||
} | ||
|
||
close(s.noDebugProcess.exited) | ||
s.logToConsole(proc.ErrProcessExited{Pid: cmd.ProcessState.Pid(), Status: cmd.ProcessState.ExitCode()}.Error()) | ||
s.send(&dap.TerminatedEvent{Event: *newEvent("terminated")}) | ||
}() | ||
return | ||
} | ||
|
||
if redirected { | ||
redirector, err := NewRedirector() | ||
if err != nil { | ||
s.sendShowUserErrorResponse(request.Request, InternalError, "Internal Error", | ||
fmt.Sprintf("failed to generate stdio pipes - %v", err)) | ||
return | ||
} | ||
|
||
s.config.Debugger.Redirect = redirector | ||
s.wg.Add(1) | ||
go func() { | ||
defer s.wg.Done() | ||
if err = redirector.ReadRedirect("stdout", func(reader io.Reader) { | ||
readerFunc(reader, "stdout") | ||
}); err != nil { | ||
s.sendShowUserErrorResponse(request.Request, InternalError, "Internal Error", | ||
fmt.Sprintf("failed to open stdout pipe - %v", err)) | ||
return | ||
} | ||
|
||
}() | ||
s.wg.Add(1) | ||
go func() { | ||
defer s.wg.Done() | ||
if err = redirector.ReadRedirect("stderr", func(reader io.Reader) { | ||
readerFunc(reader, "stderr") | ||
}); err != nil { | ||
s.sendShowUserErrorResponse(request.Request, FailedToLaunch, "Failed to launch", | ||
fmt.Sprintf("failed to open stderr pipe - %v", err)) | ||
return | ||
} | ||
}() | ||
} | ||
|
||
func() { | ||
s.mu.Lock() | ||
defer s.mu.Unlock() // Make sure to unlock in case of panic that will become internal error | ||
|
@@ -1082,15 +1211,30 @@ func (s *Session) getPackageDir(pkg string) string { | |
|
||
// newNoDebugProcess is called from onLaunchRequest (run goroutine) and | ||
// requires holding mu lock. It prepares process exec.Cmd to be started. | ||
func (s *Session) newNoDebugProcess(program string, targetArgs []string, wd string) (*exec.Cmd, error) { | ||
func (s *Session) newNoDebugProcess(program string, targetArgs []string, wd string, redirected bool) (cmd *exec.Cmd, err error) { | ||
if s.noDebugProcess != nil { | ||
return nil, fmt.Errorf("another launch request is in progress") | ||
} | ||
cmd := exec.Command(program, targetArgs...) | ||
cmd.Stdout, cmd.Stderr, cmd.Stdin, cmd.Dir = os.Stdout, os.Stderr, os.Stdin, wd | ||
if err := cmd.Start(); err != nil { | ||
|
||
cmd = exec.Command(program, targetArgs...) | ||
aarzilli marked this conversation as resolved.
Show resolved
Hide resolved
|
||
cmd.Stdin, cmd.Dir = os.Stdin, wd | ||
|
||
if redirected { | ||
if s.stderrReader, err = cmd.StderrPipe(); err != nil { | ||
return nil, err | ||
} | ||
|
||
if s.stdoutReader, err = cmd.StdoutPipe(); err != nil { | ||
return nil, err | ||
} | ||
} else { | ||
cmd.Stdout, cmd.Stderr = os.Stdin, os.Stderr | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. should be |
||
} | ||
|
||
if err = cmd.Start(); err != nil { | ||
return nil, err | ||
} | ||
|
||
s.noDebugProcess = &process{Cmd: cmd, exited: make(chan struct{})} | ||
return cmd, nil | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Still TBD.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you mean
WriterFiles [3]*os.File -> StdinWriter *os.File, StdoutWriter *os.File, StderrWriter *os.File
?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this should just be a string/io.Writer pair:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about this?
I can't use
io.Writer
orio.WriteColser
, because ingdbserial/rr.go
it needs*os.File
.In
service/service.go
, the data will be read from this structure.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@aarzilli Need a reply.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What you suggested is ok.