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

Add close() method to connectionPool so connections are closed when origins are reloaded #551

Merged
merged 8 commits into from
Dec 4, 2019

Conversation

dvlato
Copy link
Contributor

@dvlato dvlato commented Dec 2, 2019

We were missing a ConnectionPool.close() method that will close open connections when an origins file is reloaded.

We are just invoking Connection::close() without modifying the metrics as, in most cases (except when the origin is removed), they would be overridden by the new SimpleConnectionPool for that origin server. We can discuss this to see what's the best way to implement it.

We can also discuss what's the best meaningful test we can implement. I guess we could add a global counter for the tota number of connections opened / closed (not per origin but global!). We might also have to locate all the code paths that should trigger a pool closure and add assertions for those in unit tests.

Of course, this PR does not prevent us from leaving connections pool that hadn't been returned to the pool (they are not in "availableConnections") at the moment the pool was closed.

@mikkokar
Copy link
Contributor

mikkokar commented Dec 3, 2019

Hi @dvlato this PR doesn't have any effect unless the pool consumer calls the close() method.

We need to check the following pool consumers:

  1. BackendServicesProxy & OriginsInventory - This appears to close the pool correctly.
  2. HostProxy - This is also okay
  3. ProxyToBackend class

Going forward, the HostProxy is the only class that will survive our refactored Styx Core. The other two/three will disappear. Hopefully soon.

@mikkokar
Copy link
Contributor

mikkokar commented Dec 3, 2019

Of course, this PR does not prevent us from leaving connections pool that hadn't been returned to the pool (they are not in "availableConnections") at the moment the pool was closed.

I suggest: Add a closed flag in the connection pool. Set it true when the pool is closed. Then in the returnConnection check the flag and close the returned connection when the pool is closed:

    public boolean returnConnection(Connection connection) {
        borrowedCount.decrementAndGet();

        // This is the new bit:
        if (closed) {
           connection.close();
        } 

        if (connection.isConnected()) {
            queueNewConnection(connection);
        }
        return false;
    }

Of course we may have to take extra care to ensure metrics are correctly maintained.

Copy link
Contributor

@mikkokar mikkokar left a comment

Choose a reason for hiding this comment

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

Many thanks for this @dvlato. Very helpful :-D

Left a quick comment regarding borrowed connections. Please have a look.

Approved this regardless.

@mikkokar
Copy link
Contributor

mikkokar commented Dec 4, 2019

Travis fails due to check style error:

[�[1;31mERROR�[m] src/main/java/com/hotels/styx/client/connectionpool/SimpleConnectionPool.java:[99,29] (whitespace) ParenPad: '(' is followed by whitespace.

@dvlato dvlato merged commit 9da2fd3 into ExpediaGroup:master Dec 4, 2019
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.

3 participants