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

Properly close UDPSocket before creating a new one #142

Closed
wants to merge 1 commit into from

Conversation

zachmccormick
Copy link
Contributor

Unless you properly call close() on the UDPSocket object, unclosed
sockets will very quickly accumulate. If you are sending stats to
StatsD quickly, then you may actually run out of free ports. This
changes the retry behavior to close the port before trying to open a new
one.

Unless you properly call `close()` on the UDPSocket object, unclosed
sockets will very quickly accumulate. If you are sending stats to
StatsD quickly, then you may actually run out of free ports. This
changes the retry behavior to close the port before trying to open a new
one.
@kbogtob
Copy link
Contributor

kbogtob commented Apr 7, 2020

Hello @zachmccormick! Thanks again for raising the issue and contributing to the project.

Could you allow me to push on Appboy:fix/issue-141 in order to fix the tests on your PR? Here's the diff anyway if you want to commit it yourself:

diff --git a/spec/integrations/connection_edge_case_spec.rb b/spec/integrations/connection_edge_case_spec.rb
index de7cd3f..2a15012 100644
--- a/spec/integrations/connection_edge_case_spec.rb
+++ b/spec/integrations/connection_edge_case_spec.rb
@@ -74,6 +74,9 @@ describe 'Connection edge cases test' do
         allow(fake_socket)
           .to receive(:send)
           .and_raise(IOError.new('closed stream'))
+
+        allow(fake_socket)
+          .to receive(:close)
       end

       context 'when retrying is working' do
@@ -85,6 +88,13 @@ describe 'Connection edge cases test' do
           subject.write('foobar')
         end

+        it 'close the initial socket' do
+          expect(fake_socket)
+            .to receive(:close)
+
+          subject.write('foobar')
+        end
+
         it 'retries on the second opened socket' do
           expect(fake_socket_retry)
             .to receive(:send)
@@ -140,6 +150,9 @@ describe 'Connection edge cases test' do
         allow(fake_socket)
           .to receive(:send)
           .and_raise(Errno::ECONNREFUSED.new('closed stream'))
+
+        allow(fake_socket)
+          .to receive(:close)
       end

       context 'when retrying is working' do
@@ -151,6 +164,13 @@ describe 'Connection edge cases test' do
           subject.write('foobar')
         end

+        it 'close the initial socket' do
+          expect(fake_socket)
+            .to receive(:close)
+
+          subject.write('foobar')
+        end
+
         it 'retries on the second opened socket' do
           expect(fake_socket_retry)
             .to receive(:send)

@kbogtob
Copy link
Contributor

kbogtob commented Apr 9, 2020

PR Merged through #143 (which just add tests)

@kbogtob kbogtob closed this Apr 9, 2020
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.

2 participants