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

Set channels and allocator to nil in shutdown #172

Merged
merged 4 commits into from
Feb 9, 2023

Conversation

lukebakken
Copy link
Contributor

@lukebakken lukebakken commented Feb 8, 2023

Follow up to #171

cc @Zerpet

cc @jxsl13 since it may be interesting

@lukebakken lukebakken requested a review from Zerpet February 8, 2023 18:05
@lukebakken lukebakken added this to the 1.7.0 milestone Feb 8, 2023
@lukebakken lukebakken self-assigned this Feb 8, 2023
@jxsl13
Copy link

jxsl13 commented Feb 8, 2023

will run some tests tomorrow to see how this behaves.

@jxsl13
Copy link

jxsl13 commented Feb 9, 2023

I think due to the slash in the branch name, it's not trivially possible to go get this branch as dependency:

go: github.com/rabbitmq/amqp091-go@lukebakken/nil-for-cleanup: invalid version: version "lukebakken/nil-for-cleanup" invalid: disallowed version string

Edit: using the hash did the job:

go get github.com/rabbitmq/amqp091-go@2d5555937d87f8c64e4d19a76ac6c8d0f4d7c77d
go: downloading github.com/rabbitmq/amqp091-go v1.6.2-0.20230208180231-2d5555937d87
go: upgraded github.com/rabbitmq/amqp091-go v1.6.2-0.20230208132010-0ecb414c61ad => v1.6.2-0.20230208180231-2d5555937d87

@jxsl13
Copy link

jxsl13 commented Feb 9, 2023

tests still work.

After 2d55559, we are setting some fields to nil in the shutdown
sequence. We have to ensure that the library does not panic as a
consequence, after the connection is closed.

Signed-off-by: Aitor Perez Cedres <[email protected]>
Copy link
Contributor

@Zerpet Zerpet left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added a test case to ensure that Connection.Channel() won't panic, when called on a closed connection. A user may try to use a closed connection. In such case, the library should return an error (not panic).

@Zerpet
Copy link
Contributor

Zerpet commented Feb 9, 2023

@lukebakken happy with the code as is. Feel free to add your review to my test case before merging :)

@lukebakken
Copy link
Contributor Author

@jxsl13 - I'll keep that in mind for future PRs (don't use a slash). Thanks for testing.

@jxsl13
Copy link

jxsl13 commented Feb 9, 2023

Thanks for maintaining the library :)

@Zerpet
Copy link
Contributor

Zerpet commented Feb 9, 2023

GitHub Actions is having a bad day and has queued the CodeQL action for 35 minutes (and counting). Given that previous commits passed the CI ✅ And that main is also green, I'm going to merge as-is.

@Zerpet Zerpet merged commit 2f52ea1 into main Feb 9, 2023
@Zerpet Zerpet deleted the lukebakken/nil-for-cleanup branch February 9, 2023 16:02
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

Successfully merging this pull request may close these issues.

3 participants