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 public API for clearing Seahorse::Client::NetHttp::ConnectionPool #1438

Closed
iamatypeofwalrus opened this issue Mar 1, 2017 · 3 comments
Closed

Comments

@iamatypeofwalrus
Copy link
Contributor

@iamatypeofwalrus iamatypeofwalrus commented Mar 1, 2017

Metadata
SDK Version: 2.3.x
Platforms: RHEL5, Amazon Linux

Issue:
When forking on linux based systems race conditions can arise between a parent process, child processes, and long-lived connections in Seahorse::Client::NetHttp::ConnectionPool. If a connection exists in the pool before a parent forks the child will also have access to this file descriptor and can possibly attempt to send a request on the connection at the same time another process is listening for a response.

SQS Example:

  1. Parent process long polls Aws::SQS::Client#receive_messages and receives a batch of messages
  2. Parent forks a child process for each message
    3a. Parent process long polls Aws::SQS::Client#receive_messages
    3b. Child process finishes processing and calls Aws::SQS::Message.delete

It is possible for 3a and 3b to utilize the same open file descriptor available in the Seahorse::Client::NetHttp::ConnectionPool and cause some unrelated error to be thrown.

Creating new instances of a client in the child process will not clear the ConnectionPool since all of the pools are stored in a class variable.

I created an example script that showcases this issue and it is based on a similar application that my team uses in production to process async jobs.

I was able to fix this by adding the following code after any fork:

Seahorse::Client::NetHttp::ConnectionPool.pools.each do |pool|
  pool.empty!
end

As far as I can tell from the public documentation consumers are discouraged from relying on implementation details inside of Seahorse.

Feature Request:
Add a public API for clearing connection pools (Aws.empty_connection_pools!?)

Documentation Request:
Add documentation around fork-ing best practices with respect to the Aws SDK

@awood45 awood45 added the v2 label Mar 3, 2017
@awood45

This comment has been minimized.

Copy link
Member

@awood45 awood45 commented Mar 3, 2017

Thanks for raising this. I think these requests make sense and I'm happy to either take a PR, and/or treat this as a relatively high priority feature request.

@iamatypeofwalrus

This comment has been minimized.

Copy link
Contributor Author

@iamatypeofwalrus iamatypeofwalrus commented Mar 7, 2017

I'd like to take a stab at a pull request. Should land sometime this week.

@awood45

This comment has been minimized.

Copy link
Member

@awood45 awood45 commented Apr 6, 2017

This will go out with the next release.

@awood45 awood45 closed this Apr 6, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
3 participants
You can’t perform that action at this time.