Skip to content
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

How to gracefully close connections blocked on reading from Redis streams? #933

Closed
RussellLuo opened this issue Dec 12, 2018 · 4 comments
Closed

Comments

@RussellLuo
Copy link

Question

Per the documentation of XREAD:

In order to avoid polling at a fixed or adaptive interval the command is able to block if it could not return any data, according to the specified streams and IDs, and automatically unblock once one of the requested keys accept data.

In other words, it's recommended to use the blocking mode while reading from Redis streams.

When using go-redis, we may write the following code:

package main

import (
	"log"
	"os"
	"os/signal"
	"sync"

	"github.com/go-redis/redis"
)

func main() {
	client := redis.NewClient(&redis.Options{
		Addr: "localhost:6379",
		// Only need 1 connection for reading.
		PoolSize: 1,
	})

	var waitGroup sync.WaitGroup
	waitGroup.Add(1)
	go func() {
		defer waitGroup.Done()

		lastID := "0-0"

		for {
			streams, err := client.XRead(&redis.XReadArgs{
				Streams: []string{"mystream", lastID},
				Block:   0, // Wait for new messages without a timeout.
			}).Result()
			if err != nil {
				log.Printf("err: %+v\n", err)
				return
			}

			log.Printf("received streams: %+v\n", streams)

			messages := streams[0].Messages
			lastID = messages[len(messages)-1].ID
		}
	}()

	c := make(chan os.Signal, 1)
	signal.Notify(c, os.Interrupt)

	select {
	case <-c:
		// Question: how to gracefully close the blocking connection here?
		client.Close()

		waitGroup.Wait()
	}
}

Then there is a question: how to gracefully close the blocking connection?

The problem of client.Close()

In the example code, If we use client.Close() to close the connection, it will cause an error log:

err: read tcp 127.0.0.1:55520->127.0.0.1:6379: use of closed network connection

What happened behind client.Close() is as follows:

  1. client.Close() will close the connection pool
  2. then the only 1 connection in the pool will be actually closed
  3. then client.XRead(), which is blocking on cn.WithReader(), will be unblocked and returns an error "use of closed network connection"
  4. then c.releaseConn() will be called, and err is judged as a bad one, which will finally closes the connection again

The real problem is:

  • when the connection is closed at step 2, the file descriptor is also released
  • when the connection is closed again at step 4, the original file descriptor may have been used by another valid connection, which will be closed mistakenly.

There was a discussion about the same issue on StackOverflow.

Possible solution

Use TCPConn.CloseRead or UnixConn.CloseRead, which are the Golang's equivalents of Unix's shutdown().

Then we need to add a functionCloseRead() into redis.Client.

Any other thoughts?

@vmihailenco
Copy link
Collaborator

vmihailenco commented Dec 13, 2018

Do you suggest we use CloseRead when closing the connection pool?

The alternative is to add some guard against closing the connection twice which should be trivial and conceptually simpler.

@RussellLuo
Copy link
Author

Do you suggest we use CloseRead when closing the connection pool?

In the example scenario I mentioned above, if we can avoid closing the connection twice, I think it's ok to use the existing client.Close().

There is another scenario, in theory, where a connection may be shared by two processes (e.g. both are blocked on reading from the same connection). Then client.CloseRead() can unblock both processes, while client.Close() can only unblock the process where it is called within.

That being said, I have little experience with the latter scenario in Golang, so I don't know if it's a practical problem.

The alternative is to add some guard against closing the connection twice which should be trivial and conceptually simpler.

Yeah, for the example scenario, I think the alternative solution is good. Maybe we can check the error string "use of closed network connection" (which is an internal error ErrNetClosing) before closing the connection?

@vmihailenco
Copy link
Collaborator

This issue suggests that closing fd twice is fine in most cases. We still can add some checks but it looks like there is no real problem here .

@RussellLuo
Copy link
Author

Yeah, I agree. The problem of Unix's close:

It is probably unwise to close file descriptors while they may be in use by system calls in other threads in the same process. Since a file descriptor may be reused, there are some obscure race conditions that may cause unintended side effects.

is due to its manipulation on the raw fd.

But in Go, the raw id is wrapped in an implementation of Conn (e.g. TCPConn), which will do some check to ensure the safety to be closed twice or more.

@vmihailenco Thanks for your reply. I'm going to close this issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants