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

Support idle connection timeout with pending sockets #2213

Merged
merged 3 commits into from
Jul 28, 2023

Conversation

JamesNK
Copy link
Member

@JamesNK JamesNK commented Jul 27, 2023

Fixes #2210

I think the issue is caused by a race when there is a long time interval between a channel connecting and a request being made:

  1. When a request is made, the socket is double-checked to be healthy before it's given to SocketsHttpHandler to be turned into a connection.
  2. After the health check, the server/proxy closes what it thinks is an idle socket.
  3. The handler then uses the closed socket to send a request and fails.

The prospective fix is to close and recreate the socket if it exceeds the idle connection timeout. Default of 1 minute.

@JamesNK
Copy link
Member Author

JamesNK commented Jul 27, 2023

cc @jhyuzhijian @kalduzov

@jhyuzhijian
Copy link

cc @jhyuzhijian @kalduzov

#2210 the issue behavior
.net client request java server no problem
.net client request .net server has problem
so i think close and recreate the socket if it exceeds the idle connection timeout. Default of 1 minute can not resove this question

@JamesNK
Copy link
Member Author

JamesNK commented Jul 28, 2023

@jhyuzhijian
Copy link

all right,i will test it

@kalduzov
Copy link

We currently have 3 services in testing. On two of them, such an error occurs and they do not actively load the channel. Now about 1 error per hour. The third one is actively working with Grpc and there have been no such errors in the last 24 hours.

This partly confirms what you're thinking @JamesNK .

Copy link
Contributor

@mgravell mgravell left a comment

Choose a reason for hiding this comment

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

nit on naming (this is a "consider", not a "please change"), but: LGTM

@JamesNK JamesNK merged commit 1d12340 into grpc:master Jul 28, 2023
2 checks passed
@JamesNK JamesNK deleted the jamesnk/connection-idle-timeout branch July 28, 2023 14:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants