Skip to content

[java] JdkHttpClient - Close all websockets before shutting down the executor#12035

Merged
diemol merged 2 commits intoSeleniumHQ:trunkfrom
joerg1985:closing-websocket
May 15, 2023
Merged

[java] JdkHttpClient - Close all websockets before shutting down the executor#12035
diemol merged 2 commits intoSeleniumHQ:trunkfrom
joerg1985:closing-websocket

Conversation

@joerg1985
Copy link
Copy Markdown
Member

@joerg1985 joerg1985 commented May 14, 2023

Description

This PR will change the following things:

  • Close all websockets not only the last created
  • Close the websockets before shutting down the executor, to avoid JDK-8277969
  • close() can be called multiple times, like the javadoc of Closeable is demanding

Motivation and Context

Should help with #11868

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist

  • I have read the contributing document.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have added tests to cover my changes.
  • All new and existing tests passed.

diemol
diemol reviewed May 15, 2023
View reviewed changes
Copy link
Copy Markdown
Member

@diemol diemol left a comment

Choose a reason for hiding this comment

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

Thank you for this PR!

I think this makes sense. But I do not understand how we are closing the underlyingSocket one, or how the close method is actually closing it. Probably I am missing something.

@joerg1985
Copy link
Copy Markdown
Member Author

joerg1985 commented May 15, 2023

@diemol sending the close message initiates an orderly closure of the websocket.
from the javadoc: The sendClose method does not close this WebSocket's input. It merely closes this WebSocket's output by sending a Close message.

@diemol diemol merged commit 5c891ce into SeleniumHQ:trunk May 15, 2023
@diemol diemol mentioned this pull request May 15, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@diemol diemol diemol approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@joerg1985 @diemol