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.

Sign up for GitHub

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

Jump to bottom

Reuse TCP connections #149

Merged
merged 1 commit into from
Jan 11, 2018
Merged

Reuse TCP connections #149

merged 1 commit into from
Jan 11, 2018

Conversation

rohitpaulk
Copy link
Collaborator

Fixes #148.

@rohitpaulk rohitpaulk force-pushed the reuse-tcp-connections-off-master branch from b725454 to 6b203ad Compare January 10, 2018 10:18
@rohitpaulk rohitpaulk mentioned this pull request Jan 10, 2018
Closed
@rohitpaulk
Copy link
Collaborator Author

This took me longer than expected, the ruby API was a bit unintuitive.

Wireshark screenshots of this working:

Before:

screen shot 2018-01-10 at 2 55 16 pm

After:

screen shot 2018-01-10 at 3 36 13 pm

@rohitpaulk rohitpaulk changed the title Reuse TCP connections [WIP] Reuse TCP connections Jan 10, 2018
@rohitpaulk rohitpaulk force-pushed the reuse-tcp-connections-off-master branch from 6b203ad to bf5788d Compare January 10, 2018 10:29
rohitpaulk
rohitpaulk commented Jan 10, 2018
View reviewed changes
lib/segment/analytics/request.rb Outdated
# If `start` is not called, Ruby adds a 'Connection: close' header
# too all requests, preventing us from reusing a connection for multiple
# HTTP requests
@http.start unless @http.started?
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I added this comment because it took me a while to figure this out, and the Ruby docs don't mention it explicitly.

I'll also try raising this on the Ruby issue tracker once I've experimented a bit.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Reported upstream: https://bugs.ruby-lang.org/issues/14349

@rohitpaulk rohitpaulk force-pushed the reuse-tcp-connections-off-master branch from bf5788d to 94179b8 Compare January 10, 2018 10:32
@codecov-io
Copy link

codecov-io commented Jan 10, 2018 •
edited
Loading

Codecov Report

Merging #149 into master will increase coverage by <.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #149      +/-   ##
==========================================
+ Coverage   98.38%   98.39%   +<.01%     
==========================================
  Files           9        9              
  Lines         371      373       +2     
==========================================
+ Hits          365      367       +2     
  Misses          6        6
Impacted Files Coverage Δ
lib/segment/analytics/request.rb 100% <100%> (ø) ⬆️
lib/segment/analytics/worker.rb 100% <100%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 779cc77...94179b8. Read the comment docs.

@rohitpaulk rohitpaulk changed the title [WIP] Reuse TCP connections Reuse TCP connections Jan 10, 2018
@rohitpaulk rohitpaulk requested a review from f2prateek January 10, 2018 10:49
@f2prateek f2prateek merged commit 204d5a2 into master Jan 11, 2018
@rohitpaulk rohitpaulk deleted the reuse-tcp-connections-off-master branch June 22, 2018 11:33
@rohitpaulk rohitpaulk mentioned this pull request Dec 6, 2019
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@f2prateek f2prateek f2prateek approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@rohitpaulk @codecov-io @f2prateek