print & log port bind failures, dns-related test coverage, test improvements #239

nonrational · 2020-03-27T22:20:43Z

This PR addresses a bug discovered while triaging #237.

Currently, puma-dev only fails to start it can't bind to the configured http port. If the tls or dns goroutines fail (often because the port is already taken), they do so silently and the user is led to believe that everything worked. This change ensures that port bind failures show up in logs and/or stdout if you're running interactively.

Changes

check and print return status of http.serve and dns.serve. if these goroutines fail to bind to the specified port, it'll now be logged.
- use tomb.Tomb in dns.go to allow for easy return of errors
~~expose main.shutdown channel for better control over the puma-dev server launched in tests. This way, each test that needs to boot its own puma-dev server can correctly clean up after itself.~~ this ends up calling os.Exit(0) and we lose test failures, so pulled back on this approach.
dns lookup tests for a running puma-dev dns server (on macOS only)
dns tests that ensure we listen for tcp and udp on the configured dns port
disable high_sierra in circleci. I suspect there's still a bug in fsevents that causes the restart.txt related tests to often (but not always) fail.

Demo

Old

$ puma-dev -dns-port 53  -https-port 443
2020/03/27 18:18:28 Existing valid puma-dev CA keypair found. Assuming previously trusted.
* Directory for apps: /Users/norton/.puma-dev
* Domains: test
* DNS Server port: 53
* HTTP Server port: 9280
* HTTPS Server port: 443
! Puma dev listening on http and https

New

$ ./puma-dev -dns-port 53  -https-port 443
2020/03/27 18:18:16 Existing valid puma-dev CA keypair found. Assuming previously trusted.
* Directory for apps: /Users/norton/.puma-dev
* Domains: test
* DNS Server port: 53
* HTTP Server port: 9280
* HTTPS Server port: 443
! Puma dev running...
! HTTPS Server failed: listen tcp 127.0.0.1:443: bind: permission denied
! DNS Server failed: listen tcp 127.0.0.1:53: bind: permission denied

nonrational · 2020-03-27T22:21:14Z

.circleci/config.yml

@@ -98,7 +98,7 @@ workflows:
  version: 2
  test:
    jobs:
-      - test_high_sierra
+      # - test_high_sierra


this is only used in my fork, but high sierra is flaking a lot, so turning off for now.

nonrational · 2020-03-27T22:21:26Z

cmd/puma-dev/main_darwin.go

@@ -17,7 +17,7 @@ import (
 var (
 	fDebug    = flag.Bool("debug", false, "enable debug output")
 	fDomains  = flag.String("d", "test", "domains to handle, separate with :, defaults to test")
-	fPort     = flag.Int("dns-port", 9253, "port to listen on dns for")
+	fDNSPort  = flag.Int("dns-port", 9253, "port to listen on dns for")


give this flag variable a better name

cmd/puma-dev/main_darwin.go

nonrational · 2020-03-27T22:22:34Z

cmd/puma-dev/main_darwin.go

+	go func() {
+		if err := dns.Serve(domains); err != nil {
+			fmt.Printf("! DNS Server failed: %v\n", err)
+		}


check the return of dns.Serve

nonrational · 2020-03-27T22:22:48Z

cmd/puma-dev/main_darwin.go

+	go func() {
+		if err := http.ServeTLS(tlsSocketName); err != nil {
+			fmt.Printf("! HTTPS Server failed: %v\n", err)
+		}


check the return of http.ServeTLS

nonrational · 2020-03-27T22:26:04Z

dev/dns.go

-	Address string
+	Address   string
+	udpServer *dns.Server
+	tcpServer *dns.Server


expose pointers to the servers were running internally, so we can shut them down gracefully in tests.

also, use a constructor so these pointers exist before calling Serve.

dev/dns.go

dev/dns_test.go

nonrational · 2020-03-28T19:37:07Z

~~looking into resolving the race i created, marking at work-in-progress~~ done. instantiating the udp and tcp servers in a constructor solves the race.

==================
WARNING: DATA RACE
Read at 0x000000d442d0 by goroutine 8:
  github.com/puma/puma-dev/dev.TestServeDNS_TCP_UDP.func3()
      /home/norton/go/src/github.com/puma/puma-dev/dev/dns_test.go:30 +0x3a
  github.com/puma/puma-dev/dev.TestServeDNS_TCP_UDP.func4.1()
      /home/norton/go/src/github.com/puma/puma-dev/dev/dns_test.go:41 +0x49
  github.com/puma/puma-dev/dev.TestServeDNS_TCP_UDP.func4()
      /home/norton/go/src/github.com/puma/puma-dev/dev/dns_test.go:48 +0x141
  github.com/avast/retry-go.Do()
      /home/norton/go/pkg/mod/github.com/avast/retry-go@v2.5.0+incompatible/retry.go:104 +0x2f8
  github.com/puma/puma-dev/dev.TestServeDNS_TCP_UDP()
      /home/norton/go/src/github.com/puma/puma-dev/dev/dns_test.go:34 +0x38b
  testing.tRunner()
      /home/norton/.asdf/installs/golang/1.13.8/go/src/testing/testing.go:909 +0x199

Previous write at 0x000000d442d0 by goroutine 12:
  github.com/puma/puma-dev/dev.(*DNSResponder).Serve.func1()
      /home/norton/go/src/github.com/puma/puma-dev/dev/dns.go:75 +0x172
  gopkg.in/tomb%2ev2.(*Tomb).run()
      /home/norton/go/pkg/mod/gopkg.in/tomb.v2@v2.0.0-20140626144623-14b3d72120e8/tomb.go:153 +0x4c

is there really no way to specify test-only dependencies? move unexported methods to the bottom first pass at catching errors from dns port bind refactor to use tomb over chan expose errors in listening for TLS and DNS remove default error handling. bail if we didn't get a good address remove "debug" messaging about not regenerating keypair nevermind -- most folks don't run this interactively, and its handy to have it in the log for now, just skip dns tests outside darwin fix compile error

skip high_sierra for now. fsevents are broken loop over tcp, udp go mod tidy map[string](func() interface{}) is awesome. refactor stdout capture to use return function instead of passed function call forgot one see if waiting for the shutdown to finish helps assert serve doesn't serve an error either only try to shutdown the server if we have a handle to it defer shutdown and fail if unable to shutdown remove timeout. we'll see if that fails. remove error stubs use constructor to init dns servers to avoid race remove retry print missed one shorten timeout, add comments to clarify intent of retries

only run one server per set of server test scenarios, so os.Exit will handle the cleanup put shutdown back inside main -- it calls os.Exit() which borks testing pull out additional state into each platform spec ensure we run things on completely non-standard ports missed an import

nonrational · 2020-04-03T19:39:01Z

cmd/puma-dev/main_darwin_test.go

+
+	configureAndBootPumaDevServer(t, map[string]string{
+		"dir":        appLinkDir,
+		"dns-port":   "65053",


we need platform specific server runs here, because darwin supports some flags that linux doesn't support.

nonrational · 2020-04-03T19:39:38Z

cmd/puma-dev/main_darwin_test.go

+		"https-port": "65443",
+	})
+
+	runPlatformAgnosticTestScenarios(t)


most examples are shared, but platform-specific stuff can live in each file.

nonrational · 2020-04-03T19:39:59Z

cmd/puma-dev/main_linux.go

-	stop := make(chan os.Signal, 1)
-
-	signal.Notify(stop, os.Interrupt, syscall.SIGQUIT, syscall.SIGTERM)
+	shutdown := make(chan os.Signal, 1)


just a rename here.

nonrational · 2020-04-03T19:40:53Z

cmd/puma-dev/main_test.go

-
-	assert.True(t, ok)
-	assert.False(t, exit.Success())
-}


diff is pretty useless here, since i reordered and renamed a bunch of the private methods.

nonrational · 2020-04-03T19:42:04Z

dev/dns.go

 type DNSResponder struct {
 	Address string
+	Domains []string


now a single DNSResponder instance is bound to its domains, rather than being able to call Serve multiple times with diff arguments on the same instance.

evanphx

Looks goooooooood!

nonrational commented Mar 27, 2020

View reviewed changes

nonrational changed the title ~~print & log port bind failures, dns-related test coverage, test improvements~~ [wip] print & log port bind failures, dns-related test coverage, test improvements Mar 28, 2020

nonrational added 4 commits April 3, 2020 13:33

test tcp and udp connection to dns

b58e3f1

nonrational force-pushed the log-port-bind-failures branch from 74ec30c to 8fcf220 Compare April 3, 2020 17:35

nonrational added 3 commits April 3, 2020 15:24

alphabetize main test methods, pass flags to puma-dev boot

d05d2ea

remove unused import

0c27432

remove another unused import

6a656d7

nonrational changed the title ~~[wip] print & log port bind failures, dns-related test coverage, test improvements~~ print & log port bind failures, dns-related test coverage, test improvements Apr 3, 2020

nonrational commented Apr 3, 2020

View reviewed changes

nonrational requested a review from evanphx April 3, 2020 19:44

nonrational added the bug label Apr 3, 2020

nonrational added 3 commits April 5, 2020 14:42

ensure we don't fail to boot the server, add additional dns tests

15870c2

ensure we set flags _before_ we read *fHTTPPort /facepalm

082e44b

trying to debug a really rare flake.

b1474bd

evanphx approved these changes Apr 6, 2020

View reviewed changes

nonrational merged commit b98aa54 into puma:master Apr 6, 2020

nonrational deleted the log-port-bind-failures branch April 6, 2020 17:50

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

print & log port bind failures, dns-related test coverage, test improvements #239

print & log port bind failures, dns-related test coverage, test improvements #239

nonrational commented Mar 27, 2020 •

edited

Loading

nonrational Mar 27, 2020

nonrational Mar 27, 2020

nonrational Mar 27, 2020

nonrational Mar 27, 2020

nonrational Mar 27, 2020

nonrational Apr 3, 2020

nonrational commented Mar 28, 2020 •

edited

Loading

nonrational Apr 3, 2020

nonrational Apr 3, 2020 •

edited

Loading

nonrational Apr 3, 2020

nonrational Apr 3, 2020

nonrational Apr 3, 2020 •

edited

Loading

evanphx left a comment

print & log port bind failures, dns-related test coverage, test improvements #239

print & log port bind failures, dns-related test coverage, test improvements #239

Conversation

nonrational commented Mar 27, 2020 • edited Loading

Changes

Demo

Old

New

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

nonrational commented Mar 28, 2020 • edited Loading

Choose a reason for hiding this comment

nonrational Apr 3, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nonrational Apr 3, 2020 • edited Loading

Choose a reason for hiding this comment

evanphx left a comment

Choose a reason for hiding this comment

nonrational commented Mar 27, 2020 •

edited

Loading

nonrational commented Mar 28, 2020 •

edited

Loading

nonrational Apr 3, 2020 •

edited

Loading

nonrational Apr 3, 2020 •

edited

Loading