Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

#

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # 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
# for free to join this conversation on GitHub. Already have an account? # 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