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

Remove conn.mux #173

Merged
merged 10 commits into from
Dec 13, 2022
Merged

Remove conn.mux #173

merged 10 commits into from
Dec 13, 2022

Conversation

jhendrixMSFT
Copy link
Member

conn.connReader() dispatches frames directly to sessions now.
Added conn.NextSession() and conn.DeleteSession() for deterministic
session management.
Channel numbers are now recycled immediately which prompted a fix for
TestSessionClose.
Fixed various tests to handle close frame (the error was being swallowed
before).
Tests that utilize testconn were silently failing due to a bug in
Conn.Read which has been fixed.

Fixes #164

CHANGELOG.md Outdated Show resolved Hide resolved
@jhendrixMSFT jhendrixMSFT marked this pull request as draft July 21, 2022 15:39
@richardpark-msft
Copy link
Member

There's a failure in our websockets negotiation, I have a draft PR with my branch that merges in your changes:
Azure/azure-sdk-for-go#18669

Copy link
Member

@richardpark-msft richardpark-msft left a comment

Choose a reason for hiding this comment

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

Some intermediate comments, still reviewing.

conn.go Outdated Show resolved Hide resolved
conn.go Outdated Show resolved Hide resolved
conn.go Outdated Show resolved Hide resolved
@jhendrixMSFT jhendrixMSFT reopened this Nov 4, 2022
@jhendrixMSFT jhendrixMSFT force-pushed the conn_mux branch 4 times, most recently from 9975484 to ada9e2b Compare November 7, 2022 21:24
@jhendrixMSFT jhendrixMSFT marked this pull request as ready for review November 7, 2022 21:24
@jhendrixMSFT jhendrixMSFT marked this pull request as draft November 11, 2022 18:21
@jhendrixMSFT jhendrixMSFT marked this pull request as ready for review November 14, 2022 20:15
@jhendrixMSFT jhendrixMSFT changed the base branch from mux_removal to main November 14, 2022 22:38
Copy link
Member

@richardpark-msft richardpark-msft left a comment

Choose a reason for hiding this comment

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

A few comments. There's enough here now that it's worth running the live tests against this version of go-amqp. I can walk you through that when you're ready.

conn.go Outdated Show resolved Hide resolved
conn.go Outdated Show resolved Hide resolved
conn.go Show resolved Hide resolved
conn.go Outdated Show resolved Hide resolved
conn.go Show resolved Hide resolved
conn.go Show resolved Hide resolved
@jhendrixMSFT
Copy link
Member Author

Test TestLinkFlowWithDrain hung. Interestingly I can also repro this on main.

@jhendrixMSFT
Copy link
Member Author

Test bug, fix pending.

@jhendrixMSFT
Copy link
Member Author

Switching back to draft for now as I want to wait to merge this until after the API clean-up changes are in and released.

Copy link
Member

@richardpark-msft richardpark-msft left a comment

Choose a reason for hiding this comment

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

It's really looking good to me. It's in the heart of the AMQP stack though, so we should probably come up with some more extreme cases for testing as well since SB and EH don't necessarily push the boundaries too much in normal operation.

I also left some comments in spots where there might be perf bottlenecks that we could investigate as well.

conn.go Show resolved Hide resolved
conn.go Outdated Show resolved Hide resolved
conn.go Outdated Show resolved Hide resolved
conn.go Outdated Show resolved Hide resolved
session.go Show resolved Hide resolved
conn.connReader() dispatches frames directly to sessions now.
Added conn.NextSession() and conn.DeleteSession() for deterministic
session management.
Channel numbers are now recycled immediately which prompted a fix for
TestSessionClose.
Fixed various tests to handle close frame (the error was being swallowed
before).
Tests that utilize testconn were silently failing due to a bug in
Conn.Read which has been fixed.
fixed propagation of RemoteErr on close
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.

Fix Session leak if context times out
2 participants