Skip to content

Fix blocking close method in multiplexed channel #4240

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

Conversation

Arooba-git
Copy link

@Arooba-git Arooba-git commented Jul 29, 2023

Hello! 🙂
This PR fixes a blocking case similar to #2145
While that issue was addressed by adding the blocking method to allowed blocking calls, BlockHound still detects another blocking close method in Http2MultiplexedChannelPool class:

aws-blocking aws-blocking-code

This PR fixes the call. We re-ran the tests and they passed after the fix, except the one related to interruption, which we have updated according to the changes.

In addition we compared the performance (in terms of memory usage) before and after the fix:

Before
aws-before

After
aws-after

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)

Checklist

  • I have read the CONTRIBUTING document
  • Local run of mvn install succeeds
  • My code follows the code style of this project
  • My change requires a change to the Javadoc documentation
  • I have updated the Javadoc documentation accordingly
  • I have added tests to cover my changes
  • All new and existing tests passed
  • I have added a changelog entry. Adding a new entry must be accomplished by running the scripts/new-change script and following the instructions. Commit the new file created by the script in .changes/next-release with your changes.
  • My change is to implement 1.11 parity feature and I have updated LaunchChangelog

License

  • I confirm that this pull request can be released under the Apache 2 license

@Arooba-git Arooba-git requested a review from a team as a code owner July 29, 2023 10:16
@debora-ito debora-ito added the needs-review This issue or PR needs review from the team. label Aug 3, 2023
@debora-ito
Copy link
Member

Hi @Arooba-git apologies for the delay in response.

I understand you're trying to change close() to be non-blocking, but we don't think that changing to a single thread executor is the right solution, as it would impact throughput performance.

@debora-ito debora-ito added the closing-soon This issue will close in 4 days unless further comments are made. label Aug 19, 2023
@Arooba-git
Copy link
Author

Hi @Arooba-git apologies for the delay in response.

No worries :)

we don't think that changing to a single thread executor is the right solution, as it would impact throughput performance.

I see.. though we can set its property so that its a 'daemon' thread... no? 🤔

@github-actions github-actions bot removed the closing-soon This issue will close in 4 days unless further comments are made. label Aug 20, 2023
@debora-ito
Copy link
Member

Still, we would like to avoid it. We don't have suggestions for now that can be addressed in this PR. You can create a new Github issue as a feature request if you'd like.

@debora-ito debora-ito added the response-requested Waiting on additional info and feedback. Will move to "closing-soon" in 10 days. label Aug 25, 2023
@github-actions
Copy link

It looks like this PR has not been active for more than five days. In the absence of more information, we will be closing this PR soon. Please add a comment to prevent automatic closure, or if the PR is already closed please feel free to open a new one.

@github-actions github-actions bot added closing-soon This issue will close in 4 days unless further comments are made. closed-for-staleness and removed closing-soon This issue will close in 4 days unless further comments are made. labels Aug 30, 2023
@github-actions github-actions bot closed this Sep 1, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
closed-for-staleness needs-review This issue or PR needs review from the team. response-requested Waiting on additional info and feedback. Will move to "closing-soon" in 10 days.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants