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

Ensure TestHttpChannel.close() is idempotent #96564

Merged
merged 3 commits into from
Jun 6, 2023

Conversation

thecoop
Copy link
Member

@thecoop thecoop commented Jun 5, 2023

Closeable.close states that close is idempotent - you can call it on an already-closed object and it doesn't do anything. This change makes TestHttpChannel.close() follow this contract.

@thecoop thecoop added >test Issues or PRs that are addressing/adding tests :Core/Infra/REST API REST infrastructure and utilities labels Jun 5, 2023
@elasticsearchmachine elasticsearchmachine added v8.9.0 Team:Core/Infra Meta label for core/infra team labels Jun 5, 2023
@elasticsearchmachine
Copy link
Collaborator

Pinging @elastic/es-core-infra (Team:Core/Infra)

@thecoop
Copy link
Member Author

thecoop commented Jun 5, 2023

I can see an argument for leaving it as-is - it's for testing purposes only, and close shouldn't be called multiple times, as that might indicate a bug. But it is valid to call close multiple times, so should TestHttpChannel allow it?

Copy link
Contributor

@DaveCTurner DaveCTurner left a comment

Choose a reason for hiding this comment

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

While I agree that throwing an ISE is the wrong reaction to double-closing, I'd rather we asserted that this never happens. Yes technically these things should be idempotent according to the JDK docs, but in practice we should consider it a logic error if we're double-closing stuff.

@thecoop
Copy link
Member Author

thecoop commented Jun 5, 2023

I've added an assertion failure to the close.

In practice, asserts are always turned on for tests, so this will still fail, but it turns the failure into an AssertionError which is more fitting what we want here

@thecoop thecoop force-pushed the idempotent-close branch from 445c1d2 to dc9313c Compare June 5, 2023 12:04
@thecoop thecoop requested a review from DaveCTurner June 5, 2023 12:05
Copy link
Contributor

@DaveCTurner DaveCTurner left a comment

Choose a reason for hiding this comment

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

LGTM

@thecoop
Copy link
Member Author

thecoop commented Jun 6, 2023

@elasticmachine update branch

@thecoop thecoop merged commit fe47e14 into elastic:main Jun 6, 2023
@thecoop thecoop deleted the idempotent-close branch June 6, 2023 08:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
:Core/Infra/REST API REST infrastructure and utilities Team:Core/Infra Meta label for core/infra team >test Issues or PRs that are addressing/adding tests v8.9.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants