Skip to content

Conversation

@Berrysoft
Copy link
Member

When read_tls returns Ok(0), the connection might be disconnected. We should return immediately instead of spinning.

@Berrysoft Berrysoft self-assigned this Nov 3, 2025
@Berrysoft Berrysoft added bug Something isn't working package: tls Related to compio-tls labels Nov 3, 2025
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR adds EOF detection to the TLS stream read implementation by checking if read_tls returns 0 bytes read and breaking out of the read loop in that case.

Key Changes:

  • Modified read_impl method to check for EOF condition (0 bytes returned from read_tls)

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@Berrysoft Berrysoft marked this pull request as draft November 3, 2025 10:10
@Berrysoft Berrysoft requested a review from Copilot November 3, 2025 10:49
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@Berrysoft Berrysoft requested a review from Copilot November 3, 2025 12:39
@Berrysoft Berrysoft marked this pull request as ready for review November 3, 2025 12:39
@Berrysoft
Copy link
Member Author

I propose slightly breaking changes in this PR, as an emergency bug fix. As copilot said, the previously version might be attacked because of my buggy code. I have checked that the breaking change won't break the code of cyper.

@Berrysoft Berrysoft added the package: io Related to compio-io label Nov 3, 2025
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

Copilot reviewed 9 out of 9 changed files in this pull request and generated 2 comments.

Comments suppressed due to low confidence (1)

compio-tls/src/adapter.rs:75

  • The domain.to_string() creates an unnecessary allocation when domain is already a &str. The futures_rustls::TlsConnector::connect likely accepts ServerName or similar that can be constructed directly from &str without cloning. Consider using domain.try_into() directly if the API supports it.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

George-Miao
George-Miao previously approved these changes Nov 3, 2025
Copy link
Member

@George-Miao George-Miao left a comment

Choose a reason for hiding this comment

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

LGTM

@Berrysoft
Copy link
Member Author

Forgot to update the workspace manifest:(

@Berrysoft Berrysoft merged commit 0c3360b into compio-rs:master Nov 3, 2025
50 checks passed
@Berrysoft Berrysoft deleted the fix/rustls-read branch November 3, 2025 17:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working package: io Related to compio-io package: tls Related to compio-tls

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants