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

Use a singleton threadpool for kex message handler flushing #459

Merged
merged 1 commit into from
Jan 30, 2024

Conversation

FliegenKLATSCH
Copy link
Contributor

fixes #458
Threads will expire after 60 seconds.

@tomaswolf
Copy link
Member

While the task submitted in flushQueue() quits when a new KEX has started while flushing or when the session has been closed while flushing, I wonder if we now need something to prevent a second task being submitted by the same session before the first one has ended.

@garydgregory
Copy link
Member

Should the threads be daemon threads?

@FliegenKLATSCH
Copy link
Contributor Author

FliegenKLATSCH commented Jan 29, 2024

While the task submitted in flushQueue() quits when a new KEX has started while flushing or when the session has been closed while flushing, I wonder if we now need something to prevent a second task being submitted by the same session before the first one has ended.

Good point, I'd tend to say no because of the locking in the loop.

Should the threads be daemon threads?

Actually not really, but the same is probably true for other usages as well :/ Wondering if this would be a real world issue, since implementations would anyway wait for a response. Could rather be an issue on the server side.
Should we make it configurable? I am wondering if someone knows some good framework around thread pools to be used?
Just to make it clear: They will become daemon threads with the proposed change. I think there are reasons for and against it. If we want to prevent loosing some packets on a shutdown, we could wait for the threads to be finished in a shutdown hook.

@gnodet gnodet added this to the 2.12.1 milestone Jan 30, 2024
@gnodet gnodet merged commit 7d77e6d into apache:master Jan 30, 2024
8 checks passed
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.

KeyExchangeMessageHandler threads pilling up
4 participants