Refactor NDT server #110

pboothe · 2019-05-03T19:00:29Z

This change is

coveralls · 2019-05-03T20:02:40Z

Pull Request Test Coverage Report for Build 633

262 of 341 (76.83%) changed or added relevant lines in 8 files are covered.
36 unchanged lines in 4 files lost coverage.
Overall coverage decreased (-3.6%) to 67.232%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
legacy/handler/wshandler.go	26	29	89.66%
legacy/singleserving/server.go	64	75	85.33%
legacy/plain/plain.go	83	102	81.37%
legacy/s2c/s2c.go	30	52	57.69%
legacy/c2s/c2s.go	32	56	57.14%

Files with Coverage Reduction	New Missed Lines	%
legacy/legacy.go	1	61.76%
legacy/c2s/c2s.go	4	55.88%
legacy/s2c/s2c.go	8	62.03%
legacy/protocol/protocol.go	23	70.7%

Totals
Change from base Build 603:	-3.6%
Covered Lines:	991
Relevant Lines:	1474

💛 - Coveralls

…ons.

pboothe · 2019-05-06T22:04:19Z

Part of #107 in preparation for #92

gfr10598

Reviewable status: 0 of 1 approvals obtained (waiting on @stephen-soltesz)

legacy/handler/tcphandler.go, line 37 at r2 (raw file):

// this code. In the future, if you are thinking of adding protocol sniffing to
// your system, don't.
func (rh *rawHandler) sniffThenHandle(conn net.Conn) {

FYI: Sorry to jump into a review, but I had hoped (and assumed) that we were going to move the support for the binary clients to a different port (or vice versa), instead of carrying this hack forward. Has that been abandoned? This makes me sad. 8-(

pboothe

Reviewable status: 0 of 1 approvals obtained (waiting on @stephen-soltesz)

legacy/handler/tcphandler.go, line 37 at r2 (raw file):

Previously, gfr10598 (Gregory Russell) wrote…

FYI: Sorry to jump into a review, but I had hoped (and assumed) that we were going to move the support for the binary clients to a different port (or vice versa), instead of carrying this hack forward. Has that been abandoned? This makes me sad. 8-(

They can't be moved. See the comment. What we can do (and this does do) is to have one port that only does ws, one that only does wss, and one that does raw ndt + protocol detection. Then, when the protocol detection is triggered, the connection is forwarded to the ws-only port. So the only jank is in the first few lines of sniffThenHandle.

pboothe

Reviewable status: 0 of 1 approvals obtained (waiting on @stephen-soltesz)

legacy/handler/tcphandler.go, line 37 at r2 (raw file):

Previously, pboothe (Peter Boothe) wrote…

They can't be moved. See the comment. What we can do (and this does do) is to have one port that only does ws, one that only does wss, and one that does raw ndt + protocol detection. Then, when the protocol detection is triggered, the connection is forwarded to the ws-only port. So the only jank is in the first few lines of sniffThenHandle.

See the comment right at the beginning of the protocol-sniffing, I mean.

stephen-soltesz

Reviewed 1 of 10 files at r1, 7 of 13 files at r2.
Reviewable status: 1 change requests, 0 of 1 approvals obtained (waiting on @pboothe and @stephen-soltesz)

ndt-server_test.go, line 162 at r2 (raw file):

		// Test legacy raw JSON clients
		//		{
		//			name:       "Connect with web100clt (with JSON)",

What is the plan for this test?

ndt-server_test.go, line 258 at r2 (raw file):

	wg := sync.WaitGroup{}
	// Run every test in parallel (the server must handle parallel tests just fine)
	for _, tc := range tests {

nit: maybe name the outer variable tt or similar to avoid any confusion from the masked parameter tc below?

legacy/README.md, line 12 at r2 (raw file):

functionality. If you are reading this and trying to decide how to implement
a speed test, use ndt7 and not the legacy protocol. The legacy protocol is
deprecated. It will be supported in perpetuity (or at least until usage drops

nit: I suggest saying only that it will be supported until usage drops to very low levels. And, "supported" does not distinguish between in code and in production. I would like to look forward to a day when we could put this behind a flag and leave it disabled in production.

legacy/c2s/c2s.go, line 53 at r2 (raw file):

		return 0, err
	}
	byteCount, err := testConn.DrainUntil(endTime)

In the ndt.c server and in the previous ndt-server version there were two phases of 'drain' from the client. iirc, failing to deploy that fix was the cause of the "march-2016" loss of upload tests.

Why won't this sequence cause those clients to error again?

legacy/handler/tcphandler.go, line 146 at r2 (raw file):

// connection requests that look like HTTP to a different address (assumed to be
// on the same host).
func NewTCP(forwardingaddr string) RawHandler {

Tell me if I'm making too much of this.

I'm concerned about names. The RawHandler acts like a conditional proxy. My thinking is that "TCP" is too vague. "RawHandler" is a misnomer here, and 'fowardingAddr' is the reverse proxy target.

I'm not saying I advocate these specific examples; a first pass might look like: "NewRawMuxHandler", "RawMuxHandler" , "wsAddr".

Can these be improved on?

legacy/handler/tcphandler_test.go, line 14 at r2 (raw file):

)

func Count200(w http.ResponseWriter, r *http.Request) {

This appears to be unused.

legacy/handler/tcphandler_test.go, line 26 at r2 (raw file):

		success++
	})
	forwardSrv := &http.Server{

I found this naming confusing. I expected this server to be the one that was forwarding requests. Maybe 'target' or 'backend'?

legacy/handler/tcphandler_test.go, line 31 at r2 (raw file):

	}
	rtx.Must(httpx.ListenAndServeAsync(forwardSrv), "Could not start server")
	_, err := http.Get("http://" + forwardSrv.Addr + "/test_url")

This is just a sanity check?

legacy/handler/tcphandler_test.go, line 43 at r2 (raw file):

	rtx.Must(tcpH.ListenAndServe(ctx, ":0"), "Could not start tcp server")

	rh := tcpH.(*rawHandler)

I have mixed feelings about this.

legacy/s2c/s2c.go, line 80 at r2 (raw file):

	// Send additional download results to the client.
	resultMsg := &Result{
		// TODO: clean up this logic to use socket stats rather than application-level counters.

Did the original NDT report socket stats rather than application-level counters?

legacy/singleserving/server.go, line 43 at r2 (raw file):

func (s *wsServer) ServeHTTP(w http.ResponseWriter, r *http.Request) {
	upgrader := websocket.Upgrader{

Can this use ws.Upgrader?

legacy/singleserving/server.go, line 57 at r2 (raw file):

		return
	}
	s.newConn = protocol.AdaptWsConn(wsc)

Please add a short note about what's happening with the ws hijacking followed by the server close. This looks funny if you don't know that WS takes over the connection.

legacy/singleserving/server.go, line 98 at r2 (raw file):

// StartWS starts a single-serving unencrypted websocket server. When this
// method returns without error, it is safe for a client to connect to the
// server, as the server socket will be in "listening" mode. Then returned

s/Then/The/

legacy/singleserving/server.go, line 126 at r2 (raw file):

// just a wsServer with a different ServeOnce method.
type wssServer struct {
	ws                *wsServer

Can we use "type embedding" here so that Port and Close are just inherited from wsServer?

pboothe

PTAL

Reviewable status: 1 change requests, 0 of 1 approvals obtained (waiting on @stephen-soltesz)

ndt-server_test.go, line 162 at r2 (raw file):