Skip to content

Attempt to solve the problem of thread lifecycle exceeding structure#285

Closed
jokemanfire wants to merge 1 commit intocontainerd:masterfrom
jokemanfire:server
Closed

Attempt to solve the problem of thread lifecycle exceeding structure#285
jokemanfire wants to merge 1 commit intocontainerd:masterfrom
jokemanfire:server

Conversation

@jokemanfire
Copy link
Copy Markdown
Member

@jokemanfire jokemanfire commented Feb 8, 2025

server's lifetime's problem. There's thread leak.

@jokemanfire
Copy link
Copy Markdown
Member Author

jokemanfire commented Feb 8, 2025

#284
@Tim-Zhang Is it acceptable to make modifications like this? Let the caller to manage the thread lifecycle of the server.

@jokemanfire jokemanfire changed the title (Not resolved do not merge)Attempt to solve the problem of thread lifecycle exceeding structure Attempt to solve the problem of thread lifecycle exceeding structure Feb 10, 2025
@jokemanfire jokemanfire force-pushed the server branch 2 times, most recently from a57a7ce to 2f3dee2 Compare February 10, 2025 07:04
Comment thread src/asynchronous/server.rs
Comment thread src/asynchronous/server.rs Outdated
pub async fn start(&mut self) -> Result<()> {
// In order to ensure compatibility, this interface has been added and will wait for the service thread to end indefinitely
// This will not cause thread leak.
pub async fn serve(&mut self) -> Result<()>{
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Consider consume the Server here? like tonic

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

That means the server will be block , unless you run it in a tokio task , and if you want to control the server's lifetime , the caller must think about it. So we need not control the thread lifetime now in lib.

Comment thread example/async-server.rs
Comment thread src/asynchronous/server.rs Outdated
Comment thread src/asynchronous/server.rs Outdated
}
trace!("listener exit.");
});
if do_wait{
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Why don't we discard the spawn when do_wait == true. Only stop using tokio task can solve the lifetime problem in my opinion.

Copy link
Copy Markdown
Member Author

@jokemanfire jokemanfire Feb 25, 2025

Choose a reason for hiding this comment

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

Sure, If we design it this way, we need to send signals in drop and receive signals in do-wait and try to exit the thread. If it's like now, we can maintain consistency with tonic in terms of external interface performance, and we need not to care about the task leak , to the outside world, the lifecycle of serve'struct and the tokio task is consistent, we need not to care the thread now, but both methods are feasible for me.

@jokemanfire
Copy link
Copy Markdown
Member Author

@Tim-Zhang take a look , i think this will be great.

1.add drop trait for server
2.add ut in async server
3.add it in async example

Signed-off-by: jokemanfire <hu.dingyang@zte.com.cn>
@Tim-Zhang
Copy link
Copy Markdown
Member

Hi @jokemanfire, I found a bug recently, could this #298 fix the lifetime problem?

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.

Need help. I don't know what caused the problem?

2 participants