Skip to content

Connections not being closed, even when server sets the "Connection: close" header #324

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

Closed
0xpablo opened this issue Dec 23, 2020 · 8 comments · Fixed by #328
Closed

Connections not being closed, even when server sets the "Connection: close" header #324

0xpablo opened this issue Dec 23, 2020 · 8 comments · Fixed by #328

Comments

@0xpablo
Copy link
Contributor

0xpablo commented Dec 23, 2020

Hi there,

I'm trying to perform some requests to an embedded device in my local network (by IP address). I noticed that after a few requests, the device would stop responding (requests would time out). It looks like the device supports a limited number of concurrent connections / open sockets, so after going over the limit, requests time out.

The embedded device sets the Connection: close header, but looking inside the Active Connections panel in Xcode, I notice that the connections are not being closed:


Screen Shot 2020-12-23 at 21 22 21

The only workaround I found was to shutdown the HTTPClient used to make the request and create a new one for the next request.

I'm running macOS 11, and the issue happens on both release and debug builds. My code is very similar to the sample code on the README (I don't think I'm doing anything that would cause this issue).

Any ideas?

Thanks in advance!


Environment:

- AsyncHTTPClient commit hash: 947429beb4b3f7515bfe2f1d7bb721318cf30935
- Swift version: Apple Swift version 5.3.2 (swiftlang-1200.0.45 clang-1200.0.32.28) (Version 12.3 (12C33))
- OS: macOS 11.1
@Lukasa
Copy link
Collaborator

Lukasa commented Jan 5, 2021

Do you have the full set of headers sent by the remote server? We do have code that should tear these down if Connection: close genuinely is being sent.

@0xpablo
Copy link
Contributor Author

0xpablo commented Jan 5, 2021

Sure! This is the output of a curl to that device:

curl -v  -X "POST" "http://192.168.1.116:3000/api/v1/hvac"  -d $'{
  "systemid": 1,
  "zoneid": 0
}'
Note: Unnecessary use of -X or --request, POST is already inferred.
*   Trying 192.168.1.116...
* TCP_NODELAY set
* Connected to 192.168.1.116 (192.168.1.116) port 3000 (#0)
> POST /api/v1/hvac HTTP/1.1
> Host: 192.168.1.116:3000
> User-Agent: curl/7.64.1
> Accept: */*
> Content-Type: application/json; charset=utf-8
> Content-Length: 34
> 
* upload completely sent off: 34 out of 34 bytes
< HTTP/1.1 200 OK
< Server: Airzone-Athome gateway
< Connection: close
< Access-Control-Allow-Origin: *
< Access-Control-Allow-Methods: PUT,POST,DELETE,PATCH
< Content-Type: text/html
< Content-Length: 1505
< 

@Lukasa
Copy link
Collaborator

Lukasa commented Jan 5, 2021

Given that these are plaintext, can we get a packet capture of what HTTPClient is sending and receiving? HTTPClient sends different headers than curl, and so this may well be triggering different behaviour.

@weissi
Copy link
Contributor

weissi commented Jan 18, 2021

ping @0xpablo

@0xpablo
Copy link
Contributor Author

0xpablo commented Jan 18, 2021

Sorry for the delay @Lukasa and @weissi. I recorded the traffic like this: tcpdump dst 192.168.1.116 or src 192.168.1.116 -i en8 -w trace.pcap.

trace.pcap.zip

@Lukasa
Copy link
Collaborator

Lukasa commented Jan 19, 2021

Yup, looks like something is going wrong here. I'll see if I can repro in an isolated environment.

@Lukasa
Copy link
Collaborator

Lukasa commented Jan 19, 2021

And great, ok, this is a really nasty bug: we actually leak these connections. If I spin in a loop hitting a localhost server exhibiting the same behaviour as your server, we leak connections until eventually we exhaust the FD limit and crash.

This is normally not a problem because the server should be actually closing the connection after it sends Connection: close. The draft HTTP messaging RFC update expresses this well:

A server that sends a "close" connection option MUST initiate a close of the connection (see below) after it sends the response containing "close".

This server is failing to do that. We need to defend against that case, in any rate. The best system for us is to issue a socket closure ourselves eagerly. This addresses our own resource exhaustion issue, and also solves the problem here.

I'll throw a unit test together and get this fixed.

Lukasa added a commit to Lukasa/async-http-client that referenced this issue Jan 19, 2021
Motivation:

Currently when either we or the server send Connection: close, we
correctly do not return that connection to the pool. However, we rely on
the server actually performing the connection closure: we never call
close() ourselves. This is unnecessarily optimistic: a server may
absolutely fail to close this connection. To protect our own file
descriptors, we should make sure that any connection we do not return
the pool is closed.

Modifications:

If we think a connection is closing when we release it, we now call
close() on it defensively.

Result:

We no longer leak connections when the server fails to close them.

Fixes swift-server#324.
Lukasa added a commit to Lukasa/async-http-client that referenced this issue Jan 19, 2021
Motivation:

Currently when either we or the server send Connection: close, we
correctly do not return that connection to the pool. However, we rely on
the server actually performing the connection closure: we never call
close() ourselves. This is unnecessarily optimistic: a server may
absolutely fail to close this connection. To protect our own file
descriptors, we should make sure that any connection we do not return
the pool is closed.

Modifications:

If we think a connection is closing when we release it, we now call
close() on it defensively.

Result:

We no longer leak connections when the server fails to close them.

Fixes swift-server#324.
Lukasa added a commit to Lukasa/async-http-client that referenced this issue Jan 19, 2021
Motivation:

Currently when either we or the server send Connection: close, we
correctly do not return that connection to the pool. However, we rely on
the server actually performing the connection closure: we never call
close() ourselves. This is unnecessarily optimistic: a server may
absolutely fail to close this connection. To protect our own file
descriptors, we should make sure that any connection we do not return
the pool is closed.

Modifications:

If we think a connection is closing when we release it, we now call
close() on it defensively.

Result:

We no longer leak connections when the server fails to close them.

Fixes swift-server#324.
Lukasa added a commit to Lukasa/async-http-client that referenced this issue Jan 19, 2021
Motivation:

Currently when either we or the server send Connection: close, we
correctly do not return that connection to the pool. However, we rely on
the server actually performing the connection closure: we never call
close() ourselves. This is unnecessarily optimistic: a server may
absolutely fail to close this connection. To protect our own file
descriptors, we should make sure that any connection we do not return
the pool is closed.

Modifications:

If we think a connection is closing when we release it, we now call
close() on it defensively.

Result:

We no longer leak connections when the server fails to close them.

Fixes swift-server#324.
@0xpablo
Copy link
Contributor Author

0xpablo commented Jan 19, 2021

Great!!! Thanks for investigating and the quick fix, I will try pointing to that PR to confirm that the issue is fixed, and I will also try contacting the manufacturers of the embedded device to let them know that they should be closing the socket as well.

Lukasa added a commit to Lukasa/async-http-client that referenced this issue Jan 19, 2021
Motivation:

Currently when either we or the server send Connection: close, we
correctly do not return that connection to the pool. However, we rely on
the server actually performing the connection closure: we never call
close() ourselves. This is unnecessarily optimistic: a server may
absolutely fail to close this connection. To protect our own file
descriptors, we should make sure that any connection we do not return
the pool is closed.

Modifications:

If we think a connection is closing when we release it, we now call
close() on it defensively.

Result:

We no longer leak connections when the server fails to close them.

Fixes swift-server#324.
Lukasa added a commit to Lukasa/async-http-client that referenced this issue Jan 19, 2021
Motivation:

Currently when either we or the server send Connection: close, we
correctly do not return that connection to the pool. However, we rely on
the server actually performing the connection closure: we never call
close() ourselves. This is unnecessarily optimistic: a server may
absolutely fail to close this connection. To protect our own file
descriptors, we should make sure that any connection we do not return
the pool is closed.

Modifications:

If we think a connection is closing when we release it, we now call
close() on it defensively.

Result:

We no longer leak connections when the server fails to close them.

Fixes swift-server#324.
Lukasa added a commit to Lukasa/async-http-client that referenced this issue Jan 19, 2021
Motivation:

Currently when either we or the server send Connection: close, we
correctly do not return that connection to the pool. However, we rely on
the server actually performing the connection closure: we never call
close() ourselves. This is unnecessarily optimistic: a server may
absolutely fail to close this connection. To protect our own file
descriptors, we should make sure that any connection we do not return
the pool is closed.

Modifications:

If we think a connection is closing when we release it, we now call
close() on it defensively.

Result:

We no longer leak connections when the server fails to close them.

Fixes swift-server#324.
Lukasa added a commit that referenced this issue Jan 19, 2021
Motivation:

Currently when either we or the server send Connection: close, we
correctly do not return that connection to the pool. However, we rely on
the server actually performing the connection closure: we never call
close() ourselves. This is unnecessarily optimistic: a server may
absolutely fail to close this connection. To protect our own file
descriptors, we should make sure that any connection we do not return
the pool is closed.

Modifications:

If we think a connection is closing when we release it, we now call
close() on it defensively.

Result:

We no longer leak connections when the server fails to close them.

Fixes #324.
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 a pull request may close this issue.

3 participants