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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Possible deadlock on DeferredConfirmation.Wait() #46

Closed
ismurov opened this issue Feb 23, 2022 · 3 comments
Closed

Possible deadlock on DeferredConfirmation.Wait() #46

ismurov opened this issue Feb 23, 2022 · 3 comments

Comments

@ismurov
Copy link

ismurov commented Feb 23, 2022

Hi, there. 馃憢

I'm starting to migrate from github.com/streadway/amqp package and delve into the new features of this package. Currently I want to use the PublishWithDeferredConfirm method and it doesn't look production-ready to me. The case in which the problem occurs is closing the connection while waiting for publication confirmation from the AMQP server.

What I mean in more detail:
The type Channel has a shutdown method in which resources are cleared and closed, also here calls a Close method of confirms type. But this method does not properly clean up its resources such as deferredConfirmations an awaiting DeferredConfirmation.

dc, err := c.PublishWithDeferredConfirm(exchange, key, false, false, msg)
if err != nil {
	log.Fatalf("c.PublishWithDeferredConfirm: %s", err)
}
dc.Wait() // <- deadlock

Possible solution:
Add one more method in type deferredConfirmations and call it from confirms.Close.

// Close closes all awaiting DeferredConfirmation with Nack.
func (d *deferredConfirmations) Close() error {
	d.m.Lock()
	defer d.m.Unlock()

	for k, v := range d.confirmations {
		v.confirmation = Confirmation{DeliveryTag: k, Ack: false}
		v.wg.Done()
		delete(d.confirmations, k)
	}

	return nil
}

But it seems to me that such a situation should be handled with explicit error like ErrClosed. And it's breaking change of package API.

Package version: v1.3.0

@lukebakken
Copy link
Contributor

Thank you for the report.

Possibly related to #10 and #14.

@shifengbin
Copy link
Contributor

the same reason #44

@DanielePalaia
Copy link
Contributor

PR44 and PR47 have been merged (#47). I will close this one. Keep us updated with a new Issue in case of other problems

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

4 participants