Skip to content

Fix quit message not being printed #15

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.

Sign up for GitHub

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

Merged
merged 3 commits into from
Nov 26, 2021
Merged

Fix quit message not being printed #15

Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
047122c
Fix quit message not being printed
silvanocerza Nov 24, 2021
e176a59
Add CI workflow to test Go code
silvanocerza Nov 25, 2021
91f7240
Add tests
silvanocerza Nov 25, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
27 changes: 22 additions & 5 deletions discovery.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@ import (
"regexp"
"strconv"
"strings"
"sync"

"github.com/arduino/go-properties-orderedmap"
)
Expand Down Expand Up @@ -86,6 +87,7 @@ type ErrorCallback func(err string)
type Server struct {
impl Discovery
outputChan chan *message
outputWaiter sync.WaitGroup
userAgent string
reqProtocolVersion int
initialized bool
Expand All @@ -111,8 +113,7 @@ func NewServer(impl Discovery) *Server {
// the input stream is closed. In case of IO error the error is
// returned.
func (d *Server) Run(in io.Reader, out io.Writer) error {
go d.outputProcessor(out)
defer close(d.outputChan)
d.startOutputProcessor(out)
reader := bufio.NewReader(in)
for {
fullCmd, err := reader.ReadString('\n')
Expand Down Expand Up @@ -141,8 +142,7 @@ func (d *Server) Run(in io.Reader, out io.Writer) error {
case "STOP":
d.stop()
case "QUIT":
d.impl.Quit()
d.outputChan <- messageOk("quit")
d.quit()
return nil
default:
d.outputChan <- messageError("command_error", fmt.Sprintf("Command %s not supported", cmd))
Expand Down Expand Up @@ -276,12 +276,26 @@ func (d *Server) syncEvent(event string, port *Port) {
}
}

func (d *Server) quit() {
d.impl.Quit()
d.outputChan <- messageOk("quit")
close(d.outputChan)
// If we don't wait for all messages
// to be consumed by the output processor
// we risk not printing the "quit" message.
// This may cause issues to consumers of
// the discovery since they expect a message
// that is never sent.
d.outputWaiter.Wait()
}

func (d *Server) errorEvent(msg string) {
d.outputChan <- messageError("start_sync", msg)
}

func (d *Server) outputProcessor(outWriter io.Writer) {
func (d *Server) startOutputProcessor(outWriter io.Writer) {
// Start go routine to serialize messages printing
d.outputWaiter.Add(1)
go func() {
for msg := range d.outputChan {
data, err := json.MarshalIndent(msg, "", " ")
Expand All @@ -292,5 +306,8 @@ func (d *Server) outputProcessor(outWriter io.Writer) {
}
fmt.Fprintln(outWriter, string(data))
}
// We finished consuming all messages, now
// we can exit for real
d.outputWaiter.Done()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What about dropping the channel + goroutine + waitgroup + d.quit in change of a mutex?

outputMutext.Lock()
output.Write(data)
outputMutex.Unlock()

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That would be mean refactor everything again, I'd prefer to avoid that. 😖

}()
}