when client disconnected, garbage collecting is too heavy

[Eohyung Lee]

When a client disconnect their GET object request, proxy server knows it and after 60sec(client_timeout) proxy server close their connections.(client, object-server)
Before proxy server close object-servers connection, it get garbagging collection from object-server for preventing memory leak in object-server.
(IMHO) One disconnected connection don't make problem, but if many disconnected connections in short time make heavy load in object-server.

In swift/proxy/controllers/base.py source you can see in below function. I commented my opinion.
    def close_swift_conn(self, src):
        try:
            src.swift_conn.close()
        except Exception:
            pass
        src.swift_conn = None
        try:
            while src.read(self.app.object_chunk_size): # it cause heavy load in object-server
                pass
        except Exception:
            pass
        try:
            src.close()
        except Exception:
            pass

You can test this like below

reproduce: (for heavy load in object server)

1) make large object(500MB) and upload it in swift
2) download this file with many concurrent connections (100~1000 connections)
3) after start downloading, wait for object-servers open their files.
4) then disconnect all connections.
5) you can see object server`s load are increased. (see it like loadavg)

And I tested without garbage collecting object-servers leak 4k in each connection. But it is not uncleared yet. I need more test.

[Juan J. Martínez (jjmartinez)]

I think this is the problem we're experiencing in production with swift 1.7.5.

We use the following shell line to check object-server connections vs proxy client connections:

# echo "object-server: `netstat -alnt | grep :6010 | wc -l`" ; echo "clients: `netstat -alnt | grep :443 | wc -l`";
object-server: 135
clients: 3

(in this output seems to be OK, not a problem yet)

When the object-server load gets too high, the number of connections in object-server is over 1200 and the clients is under 200.

It gets corrected by restarting the proxies.

[Juan J. Martínez (jjmartinez)]

I'm attaching a script to easily reproduce the problem. It may need some tweaking for our swift cluster.

[Juan J. Martínez (jjmartinez)]

Re attached the test scipt removing the cleanup to make it more effective.

Not it won't remove the file or the test container (cleaning was making the cluster return 404 to some GET requests instead of the intended effect).

[Juan J. Martínez (jjmartinez)]

I can't confirm there's a leak when the connection is just closed instead of reading data until the object server is done.

This is the patch I'm testing.

[Peter Portante (peter-a-portante)]

Can you propose this as a gerrit change? This seems pretty useful and would let us drop the resources on the object server.

[Juan J. Martínez (jjmartinez)]

Yes, Peter! I just didn't have the time to review the tests and see if I can add a test for this!

I'll try to submit today the change through gerrit.

[Juan J. Martínez (jjmartinez)]

In fact I'm testing the patch in 1.7.5, doesn't work like that in 1.9.0 (is _drop_cache!).

As I said, I'll submit it when all tests PASS!

[OpenStack Infra (hudson-openstack)]

Fix proposed to branch: master
Review: https://review.openstack.org/38602

[Juan J. Martínez (jjmartinez)]

I removed the patch because it was wrong. See the review request.

Btw, the problem was introduced when bug #1037337 was fixed, see:

https://github.com/openstack/swift/commit/9290471b61a98a1882f0d9e5ce7d883428e2ff36

[sirkonst (sirkonst)]

What the status of patches and problem?

I think it very important for fix, because it's simple way to make DoS attack to swift-cluster. If you make even 10 requests to 5GB file and break them, this can lead to 100% load on the internal network.

[Juan J. Martínez (jjmartinez)]

We're using that patch in production and we haven't experienced any problem so far.

Unfortunately the patch didn't get enough attention and it was abandoned after bad reviews that didn't address why the patch was rejected. I can submit it again, but I'll need help from people able to review it.

[sirkonst (sirkonst)]

DoS vulnerability must be fixed :-)

[John Dickinson (notmyname)]

I've confirmed that this affects Python 2.7 and not Python2.6.

I would have preferred that this had been submitted as a security bug initially.

[John Dickinson (notmyname)]

This is not a memory leak issue. The issue is that the proxy server (under Python2.7) will read the entire contents of the object from the storage node. When those requests are concentrated on a particular proxy or object server, the CPU spikes and prevents other requests from being handled.

[Juan J. Martínez (jjmartinez)]

Thanks John!

We are experiencing this issue with Debian Squeeze, so I can confirm it happens with 2.6.6 too; the change that introduced the bug was supposed to fix a leak on the storage nodes, but I can't confirm that.

I don't know if my proposed fix at https://review.openstack.org/38602 is OK, should I submit it again for review?

[Samuel Merritt (torgomatic)]

Well, there's a couple bad things that can happen here, and Swift needs to avoid all of them.

Bad Thing 1: when a client disconnects, the proxy can leak memory. THIS DOES NOT HAPPEN RIGHT NOW. The reason for all this connection-draining code is that there are a number of buffers between the object server and the Python code. In particular, there are buffers in C-land that, if not drained, would simply hang around forever. Obviously that *shouldn't* happen, but it did.

Bad Thing 2: when a client disconnects, the proxy reads the rest of the object for no good reason. THIS IS HAPPENING NOW. It should be fixed, and people (myself included) are looking into it.

The reason the previous patch was rejected is that, while it fixed Bad Thing 2, it removed all the workarounds for Bad Thing 1. Any patch for this issue will have to be tested very well to ensure it doesn't cause regressions. This almost certainly means automated test scripts and patches to make it easy for reviewers to both exhibit the original Bad Things *and* for reviewers to verify that the proposed patch squashes both.

[Juan J. Martínez (jjmartinez)]

I totally understand.

This bug was filled in April and it was causing a big problem in our swift deployment. Since I applied the patch in July, the memory leak either is not happening in our deployment or it is not significant.

I'll be more than happy to apply a better patch but I'm afraid I can't find a better solution myself without help.

[Samuel Merritt (torgomatic)]

FWIW, I was wrong about the memory leak: it was actually leaking socket filehandles, not memory. (At least, not leaking memory *noticeably*.)

I think I've got a fix that I'll submit shortly and see what people think.

[OpenStack Infra (hudson-openstack)]

Fix proposed to branch: master
Review: https://review.openstack.org/48538

[OpenStack Infra (hudson-openstack)]

Reviewed: https://review.openstack.org/48538
Committed: http://github.com/openstack/swift/commit/def37fb56aea7b9fe4254621e10667712052d3ac
Submitter: Jenkins
Branch: master

commit def37fb56aea7b9fe4254621e10667712052d3ac
Author: Samuel Merritt <email address hidden>
Date: Wed Sep 25 10:41:41 2013 -0700

    Stop reading from object server when client disconnects.

    If a client were in the middle of an object GET request and then
    disconnected, the proxy would wait a while (default 60s) and then time
    out the connection. As part of the teardown for this, the proxy would
    attempt to close the connection to the object server, then drain any
    associated buffers. However, this didn't work particularly well,
    resulting in the proxy reading the entire remainder of the object for
    no gain.

    Now, the proxy closes the connection hard, by calling .close() on the
    underlying socket._socket object. This is different from calling
    .close() on a socket._socketobject object, which is what you get back
    from socket.socket() and similar methods. Calling .close() on a
    socket._socketobject simply decrements a reference counter on the
    socket._socket, which has been observed in the past to result in
    socket leaks when something holds onto a reference. However, calling
    .close() on a socket._socket actually closes the socket regardless of
    who else has a reference to it.

    I had to delete a test assertion that said the object server never got
    SIGPIPE after a GET w/X-Newest. Well, you get a SIGPIPE when you write
    to a closed socket, and now the proxy is actually closing the sockets
    early, so now you *do* get a SIGPIPE.

    closes-bug: 1174660

    Note that this will cause a regression on bug 1037337; unfortunately,
    the cure is worse than the disease, so out it goes.

    Change-Id: I9c7a2e7fdb8b4232e53ea96f86b50e8d34c27221

[Lin Yun Fan (lin-yunfan)]

Hi
 I know this may not the right place for the question but it is quite similar to this problem.

I am using swift 1.74.I have a program that generate thumbnail from swift object(video),the program use http range(n-) header to get the necessary part for the thumbnail.
I found it caused swift to have alot of established connection after some debug I found the problem was caused by the client close the connection before all the content is donwloaded.
If I download part of the object then close the connection swift won't release the connection immediately and if you do that for many times the swift could too busy to handle other request

[OpenStack Infra (hudson-openstack)]

Fix proposed to branch: feature/ec
Review: https://review.openstack.org/54029

[OpenStack Infra (hudson-openstack)]

Reviewed: https://review.openstack.org/54029
Committed: http://github.com/openstack/swift/commit/94d3671b0bbf87fdbff845643963f3f9a97c58b5
Submitter: Jenkins
Branch: feature/ec

commit abcecd26a7b5871f75f0fbddf0d08bbac95bb089
Author: Kun Huang <email address hidden>
Date: Wed Oct 23 21:19:01 2013 +0800

    utf8 encode tempurl key

    In tempurl middleware, hmac uses the value of account metadata to
    generate HMAC-SHA1 signature and hmac must accept a str-type string, not
    a unicode string. The meta dict returned from get_info stroges special
    chars as unicode however. So just encode it for tempurl using.

    Closes-Bug: #1242644
    Change-Id: I4be62eea014a573efc4748470de57dccf00e431d

commit cd2e7df0b69bbd269cd3c4170e0fee8186a07c95
Author: Pete Zaitcev <email address hidden>
Date: Tue Oct 22 17:18:04 2013 -0600

    Add an __str__ method to brokers

    A few uses of broker.db_file are in printouts where we do need
    them, so the administrator may know what's up. Seems like an easy
    way to get rid of those is to make brokers identify themselves
    with common __str__. Alternative back-end implementations may
    supply something other than a filename here, for example a cluster
    name and a volume name.

    Note that I'm not sure if correct coercion would occur when
    brokers are bounced through dictionaries, hence explicit str().

    Change-Id: I329788ebd1fbe39ffadcf9f9d5194a74a88dde58

commit 9807a358c6d1314d25e3a41da75be5851fa0ac27
Author: Donagh McCabe <email address hidden>
Date: Fri Aug 23 15:03:08 2013 +0100

    Add WWW-Authenticate to 401 responses

    Per http://www.ietf.org/rfc/rfc2616.txt, when a 401 error is returned, the
    Www-Authenticate response header MUST also be returned. The format is
    described in http://www.ietf.org/rfc/rfc2617.txt.

    Swift supports and/or implements a number of authentication schemes
    including tempauth, Keystone, tempurl, formpost and container sync. In
    this fix, we use a catch-all, "Swift". The realm is the account (where
    known) or "unknown" (bad path or where the 401 is returned from code
    that does not have the request). Examples:

         Www-Authenticate: Swift realm="AUTH_1234567889"
         Www-Authenticate: Swift realm="unknown"

    Fixes bug #1215491

    Change-Id: I03362789318dfa156d3733ef9348795062a9cfc4

commit ed5101b2002b877518466ae5f9a6d652581238f2
Author: Yuan Zhou <email address hidden>
Date: Sat Oct 19 11:40:35 2013 +0800

    Adding more unit tests for audit_location_generator

    Change-Id: I40410fbbb79cea8647074f703e4675364c69d930

commit 5202b0e58613738cc81ec63e7c6da14ce5429526
Author: Peter Portante <email address hidden>
Date: Thu Sep 12 19:51:18 2013 -0400

    DiskFile API, with reference implementation

    Refactor on-disk knowledge out of the object server by pushing the
    async update pickle creation to the new DiskFileManager class (name is
    not the best, so suggestions welcome), along with the REPLICATOR
    method logic. We also move the mount checking and thread pool storage
    to the new ondisk.Devices object, which then also becomes th...

[Jeremy Stanley (fungi)]

In response to John's comment #14, there was apparently a separate security bug filed shortly before this one which seems to probably be a duplicate. I've opened and marked it as such, but do you think we need to consider retroactively issuing a security advisory about the issue?

[Thierry Carrez (ttx)]

Given the versions affected and how much time has passed since this made it in, I'd consider this a performance improvement (making DoS attacks less efficient) and not issue a retroactive advisory about it...

[Jeremy Stanley (fungi)]

Agreed, according to bug 1166198 the introduction and resolution timeline suggests this only affected folsom and grizzly.