Skip to content

Conversation

@functioner
Copy link
Contributor

@functioner functioner commented Nov 16, 2021

A patch for KAFKA-13457

Committer Checklist (excluded from commit message)

  • Verify design and implementation
  • Verify test coverage and CI build status
  • Verify documentation (including upgrade notes)

Copy link
Member

@showuon showuon left a comment

Choose a reason for hiding this comment

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

@functioner , thanks for finding this issue and fixing it. LGTM! Left a minor comment. Thanks.

Copy link
Member

@dajac dajac left a comment

Choose a reason for hiding this comment

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

@functioner Thanks for the patch.

It seems that the fix does not fully match the description in the JIRA. In the JIRA, you wrote the following: However, line 717 or the socketChannel operations within the try block may potentially throw an IOException as well, which is not handled.. And you refers to this line:

val socketChannel = serverSocketChannel.accept()     // line 717

The try/catch block where the IOException has been added does not wrap that line so it does not catch it. I suppose that the description of the JIRA is incorrect in this case because we can't close the socketChannel if we don't have it.

However, it seems to be that such issue could be caused by socketChannel.configureBlocking(false) which could also thrown an IOException. Am I getting this right?

Do you think that we could add a unit test for this bug?

@functioner
Copy link
Contributor Author

Are there any other issues or concern with the current patch and test? If any, I will resolve ASAP. Will this PR be merged soon by any chance?

@dajac
Copy link
Member

dajac commented Nov 22, 2021

@functioner It seems that the code does not compile. I seems to be due to 30a9085 which added an parameter to the Acceptor's constructor. Could you rebase or merge trunk into your branch?

Copy link
Member

@dajac dajac left a comment

Choose a reason for hiding this comment

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

Thanks for the update. Left a few comments.

Copy link
Member

@dajac dajac left a comment

Choose a reason for hiding this comment

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

LGTM, thanks for the patch.

@dajac dajac changed the title KAFKA-13457: socketChannel in Acceptor#accept is not closed upon IOException KAFKA-13457: SocketChannel in Acceptor#accept is not closed upon IOException Nov 24, 2021
@dajac dajac merged commit 28dcbad into apache:trunk Nov 24, 2021
@functioner functioner deleted the KAFKA-13457 branch November 24, 2021 17:32
@functioner
Copy link
Contributor Author

Thanks everybody here for the suggestions and merging the patch!

hgeraldino pushed a commit to hgeraldino/kafka that referenced this pull request Nov 29, 2021
…ception (apache#11504)

This patch ensures that SocketChannel in Acceptor#accept is closed if an IOException is thrown while the socket is configured.

Reviewers:  Luke Chen <[email protected]>, David Jacot <[email protected]>
xdgrulez pushed a commit to xdgrulez/kafka that referenced this pull request Dec 22, 2021
…ception (apache#11504)

This patch ensures that SocketChannel in Acceptor#accept is closed if an IOException is thrown while the socket is configured.

Reviewers:  Luke Chen <[email protected]>, David Jacot <[email protected]>
lmr3796 pushed a commit to linkedin/kafka that referenced this pull request Oct 30, 2023
… closed upon IOException (apache#11504)

This patch ensures that SocketChannel in Acceptor#accept is closed if an IOException is thrown while the socket is configured.

Reviewers:  Luke Chen <[email protected]>, David Jacot <[email protected]>
lmr3796 pushed a commit to linkedin/kafka that referenced this pull request Oct 30, 2023
… closed upon IOException (apache#11504)

This patch ensures that SocketChannel in Acceptor#accept is closed if an IOException is thrown while the socket is configured.

Reviewers:  Luke Chen <[email protected]>, David Jacot <[email protected]>
lmr3796 added a commit to linkedin/kafka that referenced this pull request Oct 30, 2023
… closed upon IOException (apache#11504) (#486)

This patch ensures that SocketChannel in Acceptor#accept is closed if an IOException is thrown while the socket is configured.

Reviewers:  Luke Chen <[email protected]>, David Jacot <[email protected]>

Co-authored-by: Haoze Wu <[email protected]>
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.

3 participants