Skip to content

Fix blocking close method in multiplexed channel#4240

Closed
Arooba-git wants to merge 4 commits intoaws:masterfrom
Arooba-git:fix-blocking-close-method-in-multiplexed-channel
Closed

Fix blocking close method in multiplexed channel#4240
Arooba-git wants to merge 4 commits intoaws:masterfrom
Arooba-git:fix-blocking-close-method-in-multiplexed-channel

Conversation

@Arooba-git
Copy link
Copy Markdown

@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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown

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