rabbit icon indicating copy to clipboard operation
rabbit copied to clipboard

The library does not allow for a timeout different from the default (30s) when connecting

Open dihedron opened this issue 1 year ago • 2 comments

It is quaite easy to implement a configurable timeout when connecting to the server: instead of this, in func New(opts):

	// try all available URLs in a loop and quit as soon as it
	// can successfully establish a connection to one of them
	for _, url := range opts.URLs {
		if opts.UseTLS {
			tlsConfig := &tls.Config{}

			if opts.SkipVerifyTLS {
				tlsConfig.InsecureSkipVerify = true
			}

			ac, err = amqp.DialTLS(url, tlsConfig)
		} else {
			ac, err = amqp.Dial(url)
		}

		if err == nil {
			// yes, we made it!
			break
		}
	}

we might do something like this:

		config := amqp.Config{
			Dial: func(network, addr string) (net.Conn, error) {
				conn, err := net.DialTimeout(network, addr, opts.ConnectionTimeout)
				if err != nil {
					return nil, err
				}

				// Heartbeating hasn't started yet, don't stall forever on a dead server.
				// A deadline is set for TLS and AMQP handshaking. After AMQP is established,
				// the deadline is cleared in openComplete.
				if err := conn.SetDeadline(time.Now().Add(opts.ConnectionTimeout)); err != nil {
					return nil, err
				}

				return conn, nil
			},
		}

		if opts.UseTLS {
			config.TLSClientConfig = &tls.Config{}
			if opts.SkipVerifyTLS {
				config.TLSClientConfig.InsecureSkipVerify = true
			}
		}

		ac, err = amqp.DialConfig(url, config)

This change entains adding ConnectionTimeout time.Duration to the options, declaring a new DefaultConnectionTimeout of 30s, and handling the lack of a sensible value in the options inside func applyDefaults().

Please let me know if you're interested in a pull request; you can check it out in my fork, in the feature/replace-logger-with-slog branch (where I'm also playing with replacing the passed in logger with log/slog - to be proposed in a separate issue).

dihedron avatar Jan 12 '25 20:01 dihedron

Thanks for the issue - yes 100% - send in a PR and let's get it in.

dselans avatar Mar 12 '25 17:03 dselans

Ok I'll work on it.

dihedron avatar Mar 13 '25 22:03 dihedron