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

KTOR-7299 Fix close race condition causing descriptor to be closed while select is still busy #4638

Merged
merged 1 commit into from
Jan 31, 2025

Conversation

Thomas-Vos
Copy link
Contributor

@Thomas-Vos Thomas-Vos commented Jan 31, 2025

KTOR-7299 IOException: Fail to select descriptor for ACCEPT

Unfortunately it does not resolve my specific issue, but I noticed this race condition when investigating the issue.
I cannot consistently reproduce this in a test so I did not add it, but I was able to reproduce it sometimes throwing kotlinx.io.IOException: Bad descriptor 5 for ACCEPT. This means the descriptor was closed multiple times, which should never happen. With the changes in this PR (and after #4637 is merged which I discovered due to this issue) it will throw kotlinx.io.IOException: Selectable closed instead. This is in line with JVM which throws ClosedChannelException (so IOException).

Explanation of race condition before this PR:

  1. Closing selectormanager closes the closeQueue:
  2. any further calls to notifyClosed will call closeDescriptor because closeQueue is already closed:
    actual fun notifyClosed(descriptor: Int) {
    if (closeQueue.addLast(descriptor)) {
    wakeupSignal.signal()
    } else {
    closeDescriptor(descriptor)
    }
    }
  3. the above could happen while the selectionLoop is still active (because wakeupSignal.signal() could take some time). That means the descriptor is closed while select is still busy!

Fixed the above issue by moving closeQueue.close() to after the selection loop has finished so descriptors are not closed while select is still busy.

@Thomas-Vos Thomas-Vos force-pushed the SelectorManager-close-issue branch from 16341e9 to 9b23adc Compare January 31, 2025 01:52
@Thomas-Vos Thomas-Vos changed the title KTOR-7299 Fix close race condition causing descriptor to be closed multiple times KTOR-7299 Fix close race condition causing descriptor to be closed while select is still busy Jan 31, 2025
Copy link
Member

@e5l e5l left a comment

Choose a reason for hiding this comment

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

Hey @Thomas-Vos, thank you for the PR. LGTM

@e5l e5l merged commit 255ce5e into ktorio:main Jan 31, 2025
15 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.

2 participants