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

net: a smart way to check internal/poll ErrTimeout #32735

Open
detailyang opened this issue Jun 22, 2019 · 14 comments

Comments

Projects
None yet
6 participants
@detailyang
Copy link

commented Jun 22, 2019

Hello guys.

It looks like we have no smart way to check the internal/poll ErrTimeout when we set the deadline on the connection.

The net.Error interface is not good enough as following

An Error represents a network error.

type Error interface {
        error
        Timeout() bool   // Is the error a timeout?
        Temporary() bool // Is the error temporary?
}

The Timeout() method may mislead caller think it's deadline error if the error is actually caused by sycall.Errno (e == EAGAIN || e == EWOULDBLOCK || e == ETIMEDOUT) rather than internal/poll ErrTimeout

Maybe we need a smart way to check the deadline error?

Many thanks in advance:)

@gopherbot gopherbot added this to the Proposal milestone Jun 22, 2019

@gopherbot gopherbot added the Proposal label Jun 22, 2019

@detailyang

This comment has been minimized.

Copy link
Author

commented Jun 22, 2019

#31449 is a good example which the TCP keepalive probe failure cause the ETIMEDOUT

@agnivade agnivade changed the title proposal: a smart way to check internal/poll ErrTimeout net: a smart way to check internal/poll ErrTimeout Jun 23, 2019

@agnivade agnivade modified the milestones: Proposal, Unplanned Jun 23, 2019

@agnivade

This comment has been minimized.

Copy link
Member

commented Jun 23, 2019

I have removed the proposal tag and made it a feature request because you have not proposed anything concrete (which a proposal should be), but you would like something like this to happen.

/cc @mikioh

@ianlancetaylor

This comment has been minimized.

Copy link
Contributor

commented Jun 23, 2019

Can you explain further why checking for and calling the Timeout method is not sufficient? Note that network descriptors will never return EAGAIN or EWOULDBLOCK in practice.

@detailyang

This comment has been minimized.

Copy link
Author

commented Jun 24, 2019

@agnivade @ianlancetaylor

I wrote a gist in Linux to trigger the keepalive probe ETIMEDOUT in https://gist.github.com/detailyang/2f5ce65321c6f97ab66babedc8d92f16

The code above is that using iptables to drop TCP keepalive probe (dport is 12345) and see kernel code in https://elixir.bootlin.com/linux/v5.2-rc5/source/net/ipv4/tcp_timer.c#L642, we see when the TCP keepalive timer failed then kernel will set the error to ETIMEDOUT

/**
 *  tcp_write_err() - close socket and save error info
 *  @sk:  The socket the error has appeared on.
 *
 *  Returns: Nothing (void)
 */

static void tcp_write_err(struct sock *sk)
{
	sk->sk_err = sk->sk_err_soft ? : ETIMEDOUT;
	sk->sk_error_report(sk);

	tcp_write_queue_purge(sk);
	tcp_done(sk);
	__NET_INC_STATS(sock_net(sk), LINUX_MIB_TCPABORTONTIMEOUT);
}
@ianlancetaylor

This comment has been minimized.

Copy link
Contributor

commented Jun 24, 2019

@detailyang Thanks, but I don't understand how that is an answer to my question. After all, you can already test for ETIMEDOUT for the cases where you need to distinguish that case.

@detailyang

This comment has been minimized.

Copy link
Author

commented Jun 24, 2019

@ianlancetaylor Yep, because I know the error is ETIMEDOUT but for some guys who expect the net.Timeout means deadline timeout, it will be confused. See #31449

@detailyang

This comment has been minimized.

Copy link
Author

commented Jun 24, 2019

People always check the deadline timeout error as the following

if err := conn.SetDeadline(time.Now().Add(1 * time.Hour)); err != nil {
    panic(err)
}

if _, err := conn.Write([]byte("test")); err != nil {
    if ne, ok := err.(net.Error); ok && ne.Timeout() {
        fmt.Println("deadline timeout)
    }
}
@networkimprov

This comment has been minimized.

Copy link

commented Jun 24, 2019

I think this is a dup of #31449, which hasn't received any attention.

@ianlancetaylor

This comment has been minimized.

Copy link
Contributor

commented Jun 24, 2019

Well, maybe.

This should be considered in conjunction with #29934, which may suggest a better approach.

@ianlancetaylor

This comment has been minimized.

Copy link
Contributor

commented Jun 24, 2019

I agree that this seems like a dup of #31449.

@detailyang

This comment has been minimized.

Copy link
Author

commented Jun 24, 2019

@ianlancetaylor yep, we need better error observability

I will keep the issue open until the #29934 or any better way land in Go

@networkimprov

This comment has been minimized.

Copy link

commented Jun 24, 2019

We don't need a new feature, we need a bugfix. Keepalive errors are connection failures, not deadline events.

@detailyang

This comment has been minimized.

Copy link
Author

commented Jun 27, 2019

PS: the comment in net.Conn it looks not right in keepalive probe failure which is

Read can be made to time out and return an Error with Timeout() == true
after a fixed time limit; see SetDeadline and SetReadDeadline.

// Multiple goroutines may invoke methods on a Conn simultaneously.
type Conn interface {
	// Read reads data from the connection.
	// Read can be made to time out and return an Error with Timeout() == true
	// after a fixed time limit; see SetDeadline and SetReadDeadline.
	Read(b []byte) (n int, err error)

	// Write writes data to the connection.
	// Write can be made to time out and return an Error with Timeout() == true
	// after a fixed time limit; see SetDeadline and SetWriteDeadline.
	Write(b []byte) (n int, err error)

	// Close closes the connection.
	// Any blocked Read or Write operations will be unblocked and return errors.
	Close() error

	// LocalAddr returns the local network address.
	LocalAddr() Addr

	// RemoteAddr returns the remote network address.
	RemoteAddr() Addr

	// SetDeadline sets the read and write deadlines associated
	// with the connection. It is equivalent to calling both
	// SetReadDeadline and SetWriteDeadline.
	//
	// A deadline is an absolute time after which I/O operations
	// fail with a timeout (see type Error) instead of
	// blocking. The deadline applies to all future and pending
	// I/O, not just the immediately following call to Read or
	// Write. After a deadline has been exceeded, the connection
	// can be refreshed by setting a deadline in the future.
	//
	// An idle timeout can be implemented by repeatedly extending
	// the deadline after successful Read or Write calls.
	//
	// A zero value for t means I/O operations will not time out.
	SetDeadline(t time.Time) error

	// SetReadDeadline sets the deadline for future Read calls
	// and any currently-blocked Read call.
	// A zero value for t means Read will not time out.
	SetReadDeadline(t time.Time) error

	// SetWriteDeadline sets the deadline for future Write calls
	// and any currently-blocked Write call.
	// Even if write times out, it may return n > 0, indicating that
	// some of the data was successfully written.
	// A zero value for t means Write will not time out.
	SetWriteDeadline(t time.Time) error
}
@hitzhangjie

This comment has been minimized.

Copy link

commented Jul 19, 2019

What's the purpose of Temporary(), when such an error will be generated?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.