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

add a MemoryManager interface to control memory allocations #96

Merged
merged 2 commits into from
Jan 17, 2022

Conversation

marten-seemann
Copy link
Contributor

No description provided.

multiplex.go Outdated
@@ -171,7 +199,10 @@ func (mp *Multiplex) CloseChan() <-chan struct{} {
}

func (mp *Multiplex) sendMsg(timeout, cancel <-chan struct{}, header uint64, data []byte) error {
buf := pool.Get(len(data) + 20)
buf, err := mp.pool.Get(len(data) + 20)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The problem here is that returning an error might not lead to a connection failure. For example, in NewStream it will just cause us to return an error and a nil stream, but not do anything with the stream that was already created.
This is not something that this PR introduced though... Still wondering if we maybe should always kill the connection when an allocation fails.

Copy link
Contributor

Choose a reason for hiding this comment

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

thats very problematic, there is no way to signal failure.

What will happen if we do nothing?
Do we leak or simply have a broken stream?
If we leak, then yes we got to kill the conn. Otherwise we can get away without doinig anything (probably), although it would be nice to be able to return an error to the other side.

Either way, up to you.


func (mp *Multiplex) getBuffer(length int) ([]byte, error) {
if err := mp.memoryManager.ReserveMemory(length, 128); err != nil {
mp.closeNoWait()
Copy link
Contributor

Choose a reason for hiding this comment

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

maybe add a comment here?

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.

2 participants