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

Fix error in the initial TLS server impl #250

Merged
merged 37 commits into from
Oct 27, 2023
Merged

Conversation

minghuaw
Copy link
Contributor

This PR fixes an oversight introduced in the earlier PR #247 which would cause the server to break out of the listening loop should there be a TLS handshake error.

Motivation

The TLS handshake was performed with the following line in #247

let stream = tls_acceptor.accept(tcp).await?;

The ? operator would return the error and end up breaking the loop.

Solution

The TLS handshake error will be logged instead of returning. The code above is replaced with

let stream = match tls_acceptor.accept(tcp).await {
    Ok(stream) => stream,
    Err(err) => {
        tracing::debug!("[VOLO] TLS handshake error: {:?}", err);
        continue;
    },
};

@minghuaw minghuaw requested review from a team as code owners October 27, 2023 06:28
@minghuaw
Copy link
Contributor Author

Is there an agreed approach on how integration tests should be set up for volo? I was thinking of adding integration tests for this kind of things but was not sure whether you guys have any preference on how it should be set up.

Copy link
Member

@PureWhiteWu PureWhiteWu left a comment

Choose a reason for hiding this comment

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

Thanks very much for this bugfix!

Is there an agreed approach on how integration tests should be set up for volo? I was thinking of adding integration tests for this kind of things but was not sure whether you guys have any preference on how it should be set up.

In fact, we have three long-running stability testing services in our production environment internally, and after any changes have been made, the services will update to the latest code.

I feel it is very important and valuable to include some integration tests directly in this repository. I would be grateful if you could help.

I've filed an issue #251 for this, maybe we can discuss more there.

@PureWhiteWu PureWhiteWu enabled auto-merge (squash) October 27, 2023 08:18
@PureWhiteWu
Copy link
Member

PTAL @Millione

@PureWhiteWu PureWhiteWu merged commit 25885a7 into cloudwego:main Oct 27, 2023
8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants