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

basichost: don't error if identify doesn't complete within deadline #2698

Closed
wants to merge 1 commit into from

Conversation

sukunrt
Copy link
Member

@sukunrt sukunrt commented Feb 1, 2024

No description provided.

@sukunrt sukunrt marked this pull request as ready for review February 1, 2024 15:48
@Stebalien
Copy link
Member

Why? This only happens if the context gets canceled, right?

@Stebalien
Copy link
Member

Or is the idea: "we have a connection, whatever"?

@sukunrt
Copy link
Member Author

sukunrt commented Feb 1, 2024

Or is the idea: "we have a connection, whatever"?

Yeah, I imagine in most cases this happens when the context times out. In which case it seems better to return the connection.
The connection is also returned when identify fails, so it seems more consistent to return the connection when identify is just slow.

@Stebalien
Copy link
Member

Well, it's the caller's context and this can also happen if:

  1. The caller cancels the request.
  2. The caller sets a really short timeout.

IMO, we should (and, IIRC, do) have a timeout for identify itself. If we hit that timeout, sure, we should return the connection. But not if the caller's timeout is canceled.

@sukunrt
Copy link
Member Author

sukunrt commented Feb 5, 2024

My problem with the API returning error is that the host is now Connected to the peer despite the fact that we return an error which is very confusing.

I'm closing this because the impact doesn't seem much especially considering we should take #2551 over the finish line.

@sukunrt sukunrt closed this Feb 5, 2024
@Stebalien
Copy link
Member

Yeah, I agree that that's annoying. But that can happen in any case so I'm not too concerned.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants