moby/spdystream

The Moby Project

Moby is an open-source project created by Docker to enable and accelerate software containerization.

It provides a "Lego set" of toolkit components, the framework for assembling them into custom container-based systems, and a place for all container enthusiasts and professionals to experiment and exchange ideas. Components include container build tools, a container registry, orchestration tools, a runtime and more, and these can be used as building blocks in conjunction with other tools and projects.

Principles

Moby is an open project guided by strong principles, aiming to be modular, flexible and without too strong an opinion on user experience. It is open to the community to help set its direction.

• Modular: the project includes lots of components that have well-defined functions and APIs that work together.
• Batteries included but swappable: Moby includes enough components to build fully featured container systems, but its modular architecture ensures that most of the components can be swapped by different implementations.
• Usable security: Moby provides secure defaults without compromising usability.
• Developer focused: The APIs are intended to be functional and useful to build powerful tools. They are not necessarily intended as end user tools but as components aimed at developers. Documentation and UX is aimed at developers not end users.

Audience

The Moby Project is intended for engineers, integrators and enthusiasts looking to modify, hack, fix, experiment, invent and build systems based on containers. It is not for people looking for a commercially supported system, but for people who want to work and learn with open source code.

Relationship with Docker

The components and tools in the Moby Project are initially the open source components that Docker and the community have built for the Docker Project. New projects can be added if they fit with the community goals. Docker is committed to using Moby as the upstream for the Docker Product. However, other projects are also encouraged to use Moby as an upstream, and to reuse the components in diverse ways, and all these uses will be treated in the same way. External maintainers and contributors are welcomed.

The Moby project is not intended as a location for support or feature requests for Docker products, but as a place for contributors to work on open source code, fix bugs, and make the code more useful. The releases are supported by the maintainers, community and users, on a best efforts basis only, and are not intended for customers who want enterprise or commercial support; Docker EE is the appropriate product for these use cases.

---

Legal

Brought to you courtesy of our legal counsel. For more context, please see the NOTICE document in this repo.

Use and transfer of Moby may be subject to certain restrictions by the United States and other governments.

It is your responsibility to ensure that your use and/or transfer does not violate applicable laws.

For more information, please see https://www.bis.doc.gov

Licensing

Moby is licensed under the Apache License, Version 2.0. See LICENSE for the full license text.

spdystream's Issues

Automatically chunk data sent to Stream.WriteData

Stream.WriteData always constructs exactly one spdy DataFrame, which causes an error when the data byte array has length greater than 16MB (the spdy max frame data size).

I'm proposing that when large (>16MB) data is sent to WriteData, we should automatically split this into multiple data frames. This change would cause a disconnect between WriteData and ReadData as currently implemented, as ReadData always expects a single data frame.

Implement flow control

Add support for window update frame

The Name/Value Header Block is missing / not verifying the Adler checksum

When generating the name/value header block for SynStream, SynReply and Header frames, the Adler checksum is not written to the stream for the ZLIB compressed data:
https://www.chromium.org/spdy/spdy-protocol/spdy-protocol-draft3-1#TOC-2.6.10-Name-Value-Header-Block

I think the problem lies in that the compress/zlib writer is not closed, only flushed:
https://github.com/docker/spdystream/blob/bc6354cbbc295e925e4c611ffe90c1f287ee54db/spdy/write.go#L240-L242

The writer writes the Adler checksum when the writer is closed, as seen here in the Close method (line 191):
https://golang.org/src/compress/zlib/writer.go

When reading a name/value header block, the checksum is ignored and not verified, which is a potential security risk. I think it doesn't get validated because the spdy reader knows exactly how many bytes to read, and it only askes the compress/zlib reader for these bytes, which means the checksum is potentially never read.
https://github.com/docker/spdystream/blob/6480d4af844c189cf5dd913db24ddd339d3a4f85/spdy/read.go#L177-L217
This is due to that the compress/zlib reader never reaches EOF, where the checksum validation occures, as seen here in the Read method (on line 96). Validation occures on line 111:
https://golang.org/src/compress/zlib/reader.go

The zlib data format is described here:
https://tools.ietf.org/html/rfc1950#section-2.2

Make spdystream.Stream implement io.WriteTo or io.ReaderFrom interface

io.Copy currently copies in chunks of 3KB for standard io.Writer and io.Reader implementations, but will instead use io.WriteTo or io.ReadFrom if the writer or reader supports the relevant method. To improve the performance of copying to/from spdystream.Stream, we can tweak the chunk size by implementing these methods.

We currently use io.Copy for spdystream.Stream in docker/libchan to handle transparent stream passing, so this could potentially improve the performance of libchan over spdy.

See io.Copy source for reference.

Inconsistent reference to license for docs

Hello,

The README.md and LICENSE.docs files are inconsistent about which license applies to the spdystream documentation.

The README.md file says that docs are:
"...licensed under the Creative Commons Attribution 4.0 International License under the terms and conditions set forth in the file "LICENSE.docs". You may obtain a duplicate copy of the same license, titled CC-BY-SA-4.0, at http://creativecommons.org/licenses/by/4.0/."

The bolded items say that the docs license is CC-BY-SA-4.0, but the italicized items say that it is CC-BY-4.0 (which is a different license).

If the intent is for the docs license to be CC-BY-4.0, could the following changes be made?

1. The LICENSE.docs file should likely be replaced with the CC-BY-4.0 license text, available from https://creativecommons.org/licenses/by/4.0/legalcode.txt

2. The bolded reference above to CC-BY-SA-4.0 in README.md should likely be changed to CC-BY-4.0.

Thank you!

Possible to send to closed channels

The code currently uses this convention to try to avoid sending to a closed channel:

select {
  case <-stream.closeChan:
    debugMessage("(%p) (%d) Data frame not sent (stream shut down)", stream, stream.streamId)
  case stream.dataChan <- frame.Data:
    debugMessage("(%p) (%d) Data frame sent", stream, stream.streamId)
}

Unfortunately, if multiple case statements can proceed, there is no guarantee that they a processed in the order they're written in the source code (see https://golang.org/ref/spec#Select_statements item 2 for more info). What this means is that it is possible for stream.closeChan to be closed, stream.dataChan to be closed, and the second case statement to be executed, resulting in a panic.

Possible to deadlock connection closure

Each time a data frame is sent, it acquires a write lock, writes the frame, and sends to resetChan. resetChan's buffer size is currently 2. If you send enough data frames quickly enough, keeping resetChan full, and the loop in monitor isn't running fast enough to drain resetChan, and the connection's closeChan closes, monitor will also attempt to acquire the write lock, but WriteFrame has it and is blocked because resetChan is full.

Put another way:

1. invoke WriteFrame, resetChan size = 1
2. invoke WriteFrame, resetChan size = 2
3. invoke WriteFrame, blocked sending to resetChan because it's full
4. connection closed
5. monitor tries to acquire write lock, but it's held by 3 above
6. deadlock since resetChan isn't being drained by anything, so 3 can't release the lock

Usage of environment variable debug=True

Problem: If one has debug=True set as environment variable, the logging behavior might be irritating using e.g. kubectl.

It brings a bunch of messages - e.g. when using the shell of an pod:

I1129 12:16:37.622982    5834 log.go:184] (0xc00010e160) (0xc0005ba1e0) Create stream
I1129 12:16:37.623601    5834 log.go:184] (0xc00010e160) (0xc0005ba1e0) Stream added, broadcasting: 1
I1129 12:16:37.716400    5834 log.go:184] (0xc00010e160) Reply frame received for 1
I1129 12:16:37.717023    5834 log.go:184] (0xc00010e160) (0xc0005ba280) Create stream
I1129 12:16:37.717043    5834 log.go:184] (0xc00010e160) (0xc0005ba280) Stream added, broadcasting: 3
I1129 12:16:37.770884    5834 log.go:184] (0xc00010e160) Reply frame received for 3
I1129 12:16:37.771082    5834 log.go:184] (0xc00010e160) (0xc000aac1e0) Create stream
I1129 12:16:37.771094    5834 log.go:184] (0xc00010e160) (0xc000aac1e0) Stream added, broadcasting: 5
I1129 12:16:37.823963    5834 log.go:184] (0xc00010e160) Reply frame received for 5
I1129 12:16:37.824279    5834 log.go:184] (0xc00010e160) (0xc00069b220) Create stream
I1129 12:16:37.824294    5834 log.go:184] (0xc00010e160) (0xc00069b220) Stream added, broadcasting: 7

Those messages appear with every key stroke when typing in the pod shell. This might be irritating to new k8s users and they might think, something is broken. I did some research on the problem - it seems like others have the same problem and a lot of questions are unanswered as its not obvious to make this link.

This was also posted in kubernetes/kubectl#1153 as I asked to change kubectl behavior.

Proposed solution: Would it be possible, to change the env variable used to something other then debug?

Ping frame

Add ping frame support

Add benchmark tests

Having benchmarks will make finding bottlenecks in the code easier. Since this is a low level library having basic benchmarks make sense. This library may be deprecated soon but with its current use is worth benchmarking for easy optimizations.

Write after Reset should return an error

Once a stream has been reset, calling Write should throw an error.

Streams should be closed when the connection is lost

If a connection is lost (lost network connectivity to other side, other side's process terminated), the Serve() loop gets an EOF and closes the Connection's closeChan, but the session's streams are not closed. If you are blocked reading from a stream, Read will not return EOF. We should close the streams when the connection is lost.

I'm working on a fix and hope to have a PR ready for review today or early next week.

Pending frames in worker queues may not be delivered after goaway frame received

If there are pending frames in the various worker queues and one of the workers processes a goaway frame, it's possible that some of the pending frames may be processed after the streams have been closed down. If you turn on debugging, you'll see 1 or more "Data frame gone away" messages after the connection's Serve() method gets the EOF from the framer trying to read frames.

A solution is to wait for all workers to drain their queues when receiving the goaway frame, and then tear down the streams.

Implement priority

Referring to http://www.chromium.org/spdy/spdy-protocol/spdy-protocol-draft3#TOC-2.3.3-Stream-priority

Calling conn.SetIdleTimeout can hang if the other end closed the connection

1. Establish SPDY connection between client and server
2. Close the client's connection
3. Attempt to call conn.SetIdleTimeout() on the server's connection

When 2 happens, the server connection's Serve goroutine ultimately exits, closing closeChan, which results in the idleAwareFramer's monitor goroutine exiting as well. When you then try to call conn.SetIdleTimeout, it attempts to send to an unbuffered setTimeoutChan, but there is no other goroutine attempting to read from that channel (it used to be monitor, but that has since exited).

I've got a local branch that uses a lock to guard SetIdleTimeout, becoming a no-op if the connection has been closed. Does that sound like a good fix to you, @dmcgowan? If so, I'll prep a PR with a test case.

Stream.IsFinished() Data Race

While running code with flag -race got this message:

==================
WARNING: DATA RACE
Write at 0x00c0006a6ed8 by goroutine 259:
  github.com/moby/spdystream.(*Stream).resetStream()
      /github.com/moby/spdystream/stream.go:199 +0x90
  github.com/moby/spdystream.(*Stream).Reset()
      /github.com/moby/spdystream/stream.go:185 +0x4f
  k8s.io/apimachinery/pkg/util/httpstream/spdy.(*connection).Close()
      /k8s.io/apimachinery/pkg/util/httpstream/spdy/connection.go:111 +0xd6

Previous read at 0x00c0006a6ed8 by goroutine 291:
  github.com/moby/spdystream.(*Stream).IsFinished()
      /github.com/moby/spdystream/stream.go:308 +0x89
==================
==================
WARNING: DATA RACE
Read at 0x00c000925f18 by goroutine 296:
  github.com/moby/spdystream.(*Stream).IsFinished()
      /github.com/moby/spdystream/stream.go:308 +0x89

Previous write at 0x00c000925f18 by goroutine 284:
  github.com/moby/spdystream.(*Connection).handleResetFrame()
      /github.com/moby/spdystream/connection.go:552 +0x14b
  github.com/moby/spdystream.(*Connection).frameHandler()
      /github.com/moby/spdystream/connection.go:428 +0x105
  github.com/moby/spdystream.(*Connection).Serve.func2()
      /github.com/moby/spdystream/connection.go:331 +0xa6
  github.com/moby/spdystream.(*Connection).Serve.func4()
      /github.com/moby/spdystream/connection.go:332 +0x47
==================

Connection can't be closed when there is unread `DataFrame` in the stream

When a stream gets a DataFrame and nothing reads data from the stream of the connection, it can't close the connection and leads to memory leak.

When I'm working on https://github.com/cri-o/cri-o memory leak issue, I found that in the situation above, spdy stream server causes memory leak.
The leak happens when it tries to close the connection, and it waits wg.Wait() forever.

spdystream/connection.go

Lines 392 to 394 in 57d1ca2

	// wait for all frame handler workers to indicate they've drained their queues
	// before handling the go away frame
	wg.Wait()

The cause of the issue is that it can't finish frameHandler() because of the deadlock.
There are some workers calling frameHandler(), and wg.Add() when each worker starts and wg.Done() when each worker is done.

spdystream/connection.go

Lines 317 to 333 in 57d1ca2

	for i := 0; i < FRAME_WORKERS; i++ {
	frameQueues[i] = NewPriorityFrameQueue(QUEUE_SIZE)
	// Ensure frame queue is drained when connection is closed
	go func(frameQueue *PriorityFrameQueue) {
	<-s.closeChan
	frameQueue.Drain()
	}(frameQueues[i])
	wg.Add(1)
	go func(frameQueue *PriorityFrameQueue) {
	// let the WaitGroup know this worker is done
	defer wg.Done()
	s.frameHandler(frameQueue, newHandler)
	}(frameQueues[i])
	}

DataFrames queued in a PriorityFrameQueue are processed in frameHandler() and if it is a DataFrame, it is handled by dataFrameHandler() ( = handleDataFrame()).
In the function, it blocks until either the stream is closed or something reads the stream dataChan (e.g. something calls Read() or ReadData())
Otherwise the function can't finish, which means frameHandler() also can't finish, and it never calls wg.Done().

spdystream/connection.go

Lines 598 to 607 in 57d1ca2

	if len(frame.Data) > 0 {
	stream.dataLock.RLock()
	select {
	case <-stream.closeChan:
	debugMessage("(%p) (%d) Data frame not sent (stream shut down)", stream, stream.streamId)
	case stream.dataChan <- frame.Data:
	debugMessage("(%p) (%d) Data frame sent", stream, stream.streamId)
	}
	stream.dataLock.RUnlock()
	}

If nothing reads the data from the stream, the only way of blocking it is to close the stream.
However streams in a connection are closed after wg.Wait().

spdystream/connection.go

Lines 394 to 409 in 57d1ca2

	wg.Wait()
	if goAwayFrame != nil {
	s.handleGoAwayFrame(goAwayFrame)
	}
	// now it's safe to close remote channels and empty s.streams
	s.streamCond.L.Lock()
	// notify streams that they're now closed, which will
	// unblock any stream Read() calls
	for _, stream := range s.streams {
	stream.closeRemoteChannels()
	}
	s.streams = make(map[spdy.StreamId]*Stream)
	s.streamCond.Broadcast()
	s.streamCond.L.Unlock()

The simplest fix is to move stream.closeRemoteChannels() before wg.Wait(), but I'm unsure if it causes other race condition errors.

please fix docs license

Docs released under Creative commons.

Is an improper grant of license. A license version should be mentioned in the proper format like "CC-BY-SA-4.0" with a link to the full text of the license ideally accompanied by full text of the license committed to repository.

Also it would be nice to mention scope of the license (i.e. which docs?).

Thanks.

Add maintainers file

spdystream depends on golang.org/x/net/spdy which no longer exists

As of 4 days ago golang.org/x/net/spdy has ceased to exist.

What's the purpose of the 10 minute wait?

In the microservice I'm building, I've experienced a memory leak using the Kubernetes client-go package, which I traced back to https://github.com/moby/spdystream/blob/master/connection.go#L733.

The memory leak is really well documented in kubernetes/kubernetes#105830.

Essentially what happens is that running the stream executors with in-cluster kubeconfig context leads to many broken pipes, which leads to the waiting time of 10 minutes before the shutdown goroutine ends. This waiting time keeps the data in memory alive. Not a big deal if it's a small number of broken pipes, but in my case the number is rising very quickly and easily can reach 2-3 GB of allocated memory within the 10-minute hold, thus considering it a memory leak of high significance (essentially a system-service pod is taking memory that the users of the cluster can't use).

The fix for this is that I replaced the waiting time from 10 minutes to 10 milliseconds instead, and the memory leak is "gone"(not truly gone, just very low and GCed quickly). However, I wonder what are the repercussions of changing https://github.com/moby/spdystream/blob/master/connection.go#L733 to milliseconds? Also, what is the point of waiting here in the first place?

module still declares it's name as github.com/moby/spdystream

error shows when trying to update dependencies:

go get: github.com/docker/[email protected] updating to github.com/docker/[email protected]: parsing go.mod: module declares its path as: github.com/moby/spdystream but was required as: github.com/docker/spdystream

obviously, the declaration comes from here

./ws_test.go:27:15: too many arguments in call to conn.Serve

When building against github.com/gorilla/websocket version 1.2.0 (latest stable supported release, june 2017) unit tests fail with:

./ws_test.go:27:15: too many arguments in call to conn.Serve
./ws_test.go:27:46: undefined: spdystream.NoAuthHandler
./ws_test.go:53:19: too many arguments in call to spdyConn.Serve
have (func(*spdystream.Stream), func(http.Header, uint8, uint32) bool)
want (spdystream.StreamHandler)
./ws_test.go:78:19: too many arguments in call to spdyConn.Serve
./ws_test.go:78:50: undefined: spdystream.RejectAuthHandler
FAIL github.com/docker/spdystream/ws [build failed]

calling conn close before stream close results in a soft deadlock

        conn, dialErr := net.Dial("tcp", LISTEN_ADDRESS)
        if dialErr != nil {
            b.Fatalf("Error dialing server: %s", dialErr)
        }

        spdyConn, spdyErr := NewConnection(conn, false)
        if spdyErr != nil {
            b.Fatalf("Error creating spdy connection: %s", spdyErr)
        }

        go spdyConn.Serve(MirrorStreamHandler)

        stream, err := spdyConn.CreateStream(http.Header{}, nil, false)

        writer := make([]byte, 1024)

        stream.Write(writer)

        if err != nil {
            panic(err)
        }

        reader := make([]byte, 1024)
        stream.Read(reader)

        closeErr := spdyConn.Close()
        if closeErr != nil {
            b.Fatalf("Error closing connection: %s, closeErr")
        }

This code should soft deadlock, or at least, that's what I was seeing in the runs I made.

if you add a stream.Close() after the stream.Read() call, it exhibits the desired behavior.

Configurable header compression

Currently the spdy framer uses the best compression setting for zlib, which is also the slowest. The compression should be configurable on connection creation to allow for less overhead during spdy stream creation. In libchan profiling switching from BestCompression to BestSpeed reduced the time overhead of compression from 24% to 4% for the same test. Since spdystream is designed to be used over any type of net.Conn, this should be configurable to match the net.Conn speed capabilities.

Changing this will involve vendoring and altering the go.net spdy package. However the spdy package appears to be frozen so I don't think that is a problem.

spdy_test.go:146: No error closing bad stream

With Go 1.12.2 on Fedora Rawhide x86_64, commit 6480d4a:

Testing    in: /builddir/build/BUILD/spdystream-6480d4af844c189cf5dd913db24ddd339d3a4f85/_build/src
         PATH: /builddir/build/BUILD/spdystream-6480d4af844c189cf5dd913db24ddd339d3a4f85/_build/bin:/builddir/.local/bin:/builddir/bin:/usr/bin:/bin:/usr/sbin:/sbin:/usr/local/sbin
       GOPATH: /builddir/build/BUILD/spdystream-6480d4af844c189cf5dd913db24ddd339d3a4f85/_build:/usr/share/gocode
  GO111MODULE: off
      command: go test -buildmode pie -compiler gc -ldflags "-X github.com/docker/spdystream/version=0 -X github.com/docker/spdystream/version.commit=6480d4af844c189cf5dd913db24ddd339d3a4f85 -extldflags '-Wl,-z,relro -Wl,--as-needed  -Wl,-z,now -specs=/usr/lib/rpm/redhat/redhat-hardened-ld '"
      testing: github.com/docker/spdystream
github.com/docker/spdystream
--- FAIL: TestSpdyStreams (0.01s)
    spdy_test.go:146: No error closing bad stream
FAIL
exit status 1
FAIL	github.com/docker/spdystream	0.272s

(The test is run in a chroot with no network access.)

I don't see the stream being closed twice in the test to actually trigger the error.

Implement GOAWAY with graceful shutdown

When a connection is closed or a GOAWAY frame is received, the spdy connection should initiate a graceful shutdown which does not allow any new streams to be created and puts each existing stream in the half closed state.

Go 1.10: spdy_bench_test.go:63: Fatalf format %s reads arg #1, but call has only 0 args

bc6354c does not pass unit tests with Go 1.10. At least:

+ GOPATH=/builddir/build/BUILD/spdystream-bc6354cbbc295e925e4c611ffe90c1f287ee54db/_build:/usr/share/gocode
+ go test -buildmode pie -compiler gc -ldflags '-extldflags '\''-Wl,-z,relro  '\'''
# github.com/docker/spdystream
./spdy_bench_test.go:63: Fatalf format %s reads arg #1, but call has only 0 args
./spdy_bench_test.go:106: Fatalf format %s reads arg #1, but call has only 0 args
FAIL    github.com/docker/spdystream [build failed]