Skip to content

Support idle connection timeout with pending sockets - #2213

Merged
JamesNK merged 3 commits into
grpc:masterfrom
JamesNK:jamesnk/connection-idle-timeout
Jul 28, 2023
Merged

JamesNK merged 3 commits into
grpc:masterfrom
JamesNK:jamesnk/connection-idle-timeout

Conversation

@JamesNK

@JamesNK JamesNK commented Jul 27, 2023

Copy link
Copy Markdown
Member

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

JamesNK commented Jul 27, 2023

Copy link
Copy Markdown
Member Author

cc @jhyuzhijian @kalduzov

@jojo-Fish

Copy link
Copy Markdown

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

JamesNK commented Jul 28, 2023

Copy link
Copy Markdown
Member Author

@jojo-Fish

Copy link
Copy Markdown

all right,i will test it

@kalduzov

Copy link
Copy Markdown

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 .

Comment thread src/Grpc.Net.Client/Balancer/Internal/SocketConnectivitySubchannelTransport.cs Outdated

@mgravell mgravell left a comment

Copy link
Copy Markdown
Contributor

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
@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