8000 refactor: use Go context to track UPF association state by LaumiH · Pull Request #122 · free5gc/smf · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

refactor: use Go context to track UPF association state #122

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 7 commits into from
Sep 25, 2024
Merged

refactor: use Go context to track UPF association state #122

merged 7 commits into from
Sep 25, 2024

Conversation

LaumiH
Copy link
@LaumiH LaumiH commented Sep 11, 2024

Right now, a UPF's association state is stored using the UPFStatus variable:

type UPFStatus int

const (
  NotAssociated          UPFStatus = 0
  AssociatedSettingUp    UPFStatus = 1
  AssociatedSetUpSuccess UPFStatus = 2
)

Whenever necessary, the association state is checked using simple logic:

if upf.UPFStatus != AssociatedSetUpSuccess {
  // do sth.
}

I think it would be better to make the UPF's association state thread-safe and improve the signaling between different functions by using a context.Context.
There already is a Ctx and CancelFunc in the UPF struct, but I don't think it is used for signaling so far.

Additionally, I suggest to improve the overall use of the context.Context Go feature in the code.
The SMFContext also has a Ctx and PFCPCancelFunc variable, which are not used for signaling in the code.

This PR improves the usage of the context.Context concept in Go in multiple parts of the code:

  1. It replaces the checks for the UPFStatus with a context.Context.Done() check. The context is called AssociationContext and replaces the old Ctx variable.
  2. It uses the SMFContext.Ctx variable more effectively. The context is renamed to SmfPfcpContext. It is the parent context for the UPF.AssociationContext.

@ianchen0119
Copy link
Contributor

Hi @LaumiH

Please help to fix the ci-linter error.

Thanks.

ianchen0119
ianchen0119 reviewed Sep 23, 2024
View reviewed changes
ianchen0119
ianchen0119 reviewed Sep 23, 2024
View reviewed changes
ianchen0119
ianchen0119 reviewed Sep 23, 2024
View reviewed changes
@ianchen0119
Copy link
Contributor

Overall, it looks good to me.

@ianchen0119
Copy link
Contributor

@Alonza0314

Please review this PR and send the feedback to me in private.
Thanks!

andy89923
andy89923 approved these changes Sep 25, 2024
View reviewed changes
Copy link
Contributor
@andy89923 andy89923 left a comment

Choose a reason for hiding this comment

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

Hi @LaumiH,

Thanks for the PR, I had rebase to main branch and test the functionality.
They works find and the code is LGTM!
Thanks!

@LaumiH LaumiH force-pushed the association-context branch from cede205 to 4631542 Compare September 25, 2024 09:08
@LaumiH
Copy link
Author
LaumiH commented Sep 25, 2024

Sorry for the second force-push, I messed with my git history -.-
It is the same code, there have been 0 changes to the commits.

@ianchen0119 ianchen0119 merged commit a700710 into free5gc:main Sep 25, 2024
3 checks passed
@ianchen0119
Copy link
Contributor

@LaumiH

Thanks for your contribution.

In addition,
I just added your profile to our contributor list

Thanks again.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ianchen0119 ianchen0119 ianchen0119 left review comments

@andy89923 andy89923 andy89923 approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@LaumiH @ianchen0119 @andy89923
0