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.

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

Experiment: A syncronous send() #75

Closed
wants to merge 1 commit into from
Closed

Experiment: A syncronous send() #75

wants to merge 1 commit into from

Conversation

mathias-baumann-frequenz
Copy link
Contributor

This little experimental PR makes the Sender.send() method syncronous by using
Event instead of Condition as I saw no need to have an extra lock at that
place.

@github-actions github-actions bot added the part:channels Affects channels implementation label Mar 2, 2023
@mathias-baumann-frequenz mathias-baumann-frequenz mentioned this pull request Mar 2, 2023
Closed
@Marenz
Make send() sync by removing lock
13dd674 
Signed-off-by: Mathias L. Baumann <mathias.baumann@frequenz.com>
@mathias-baumann-frequenz mathias-baumann-frequenz mentioned this pull request Mar 3, 2023
Merged
@ela-kotulska-frequenz
Copy link
Contributor

It is very nice, but it is not enough.
We need blocking Sender anyway: #73
So this PR should be extended to make two interfaces, for blocking and non-blocking channels.

@mathias-baumann-frequenz
Copy link
Contributor Author

Adding a send that blocks on queue full shouldn't be a problem

@mathias-baumann-frequenz
Copy link
Contributor Author

But are you sure this should be in this PR? Combining a new feature with changing an existing one?

@leandro-lucarella-frequenz leandro-lucarella-frequenz mentioned this pull request Mar 6, 2023
Open
@leandro-lucarella-frequenz
Copy link
Contributor

I created an issue for this: #76

@leandro-lucarella-frequenz
Copy link
Contributor

But are you sure this should be in this PR? Combining a new feature with changing an existing one?

Let's not change the Sender now, let's do the proper split in a separate PR, because probably all other channels senders should be sync too.

@mathias-baumann-frequenz mathias-baumann-frequenz changed the title Experiment: A syncronouse send() Experiment: A syncronous send() Apr 6, 2023
@mathias-baumann-frequenz
Copy link
Contributor Author

Closing this for now, the branch is still around.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
part:channels Affects channels implementation
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@mathias-baumann-frequenz @ela-kotulska-frequenz @leandro-lucarella-frequenz @Marenz