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

Refactor Connection._send_message in terms of _send_frame to avoid code duplication #915

Merged
merged 2 commits into from Jan 31, 2018

Conversation

Projects
None yet
3 participants
@vitaly-krugl
Member

vitaly-krugl commented Jan 10, 2018

Refactor Connection._send_message in terms of Connection._send_frame to avoid code duplication.

Also, remove default content arg default value of None since implementation assumes content is a sequence.

Renamed method arg name to method_frame to reflect the expected object type and for consistency with other methods in same module.

Fixed method comment not to refer to lock, since locking was unnecessary and was removed some time ago.

Covered by existing tests

cc @lukebakken

@codecov

This comment has been minimized.

codecov bot commented Jan 10, 2018

Codecov Report

Merging #915 into master will decrease coverage by 0.89%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master     #915     +/-   ##
=========================================
- Coverage   82.45%   81.55%   -0.9%     
=========================================
  Files          20       21      +1     
  Lines        3670     3784    +114     
  Branches      546      561     +15     
=========================================
+ Hits         3026     3086     +60     
- Misses        498      544     +46     
- Partials      146      154      +8
Impacted Files Coverage Δ
pika/connection.py 88.45% <100%> (-0.06%) ⬇️
pika/compat/__init__.py 63.71% <0%> (-20.6%) ⬇️
pika/adapters/asyncio_connection.py 58.44% <0%> (-1.3%) ⬇️
pika/adapters/base_connection.py 65.07% <0%> (-0.84%) ⬇️
pika/__init__.py 100% <0%> (ø) ⬆️
pika/adapters/libev_connection.py 73.43% <0%> (ø)
pika/adapters/select_connection.py 79.23% <0%> (+0.11%) ⬆️
pika/adapters/blocking_connection.py 84.87% <0%> (+0.13%) ⬆️
pika/adapters/__init__.py 77.27% <0%> (+5.05%) ⬆️

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 9088b23...930d8bc. Read the comment docs.

@vitaly-krugl vitaly-krugl changed the title from Refactor Connection._send_message in terms of Connection._send_frame to avoid code duplication to Refactor Connection._send_message in terms of _send_frame to avoid code duplication Jan 11, 2018

write_buffer = [frame.Method(channel_number, method).marshal(),
frame.Header(channel_number, length,
content[0]).marshal()]
self._send_frame(frame.Method(channel_number, method_frame))

This comment has been minimized.

@gmr

gmr Jan 11, 2018

Member

IIRC this approach caused problems in the past with interspersed frames due to IOLoop yielding or some such behavior? I don't remember the details, but the write buffer behavior was to make sure that multi-frame deliveries were atomic in being written.

This comment has been minimized.

@vitaly-krugl

vitaly-krugl Jan 11, 2018

Member

Hi @gmr, thanks for chiming in. I remember that too. That was before BlockingConnection was rebased on SelectConnection. That legacy blocking socket implementation used to periodically try to read incoming data in the midsts of queuing of outbound message frames which could inject frames (e.g., heartbeat ack), thus corrupting the message being sent. I fixed that as part of the big refactoring effort a few years ago.

@lukebakken lukebakken added this to the 1.0.0 milestone Jan 30, 2018

@gmr

This comment has been minimized.

Member

gmr commented Jan 31, 2018

Looks like I created a conflict in cleaning up the unit tests. Mind rebasing the tests and resolving?

# This is a combination of 2 commits.
# This is the 1st commit message:

Refactor Connection._send_message in terms of Connection._send_frame to avoid
code duplication. Also, remove default content arg value since implementation
assumes content is a sequence.

# This is the commit message #2:

Use xrange instead of range
@lukebakken

This comment has been minimized.

Contributor

lukebakken commented Jan 31, 2018

Working on those conflicts now

@lukebakken lukebakken force-pushed the vitaly-krugl:refactor-send-message branch from 027cf4a to 604f112 Jan 31, 2018

@gmr gmr merged commit 8e9257b into pika:master Jan 31, 2018

1 of 2 checks passed

continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
@vitaly-krugl

This comment has been minimized.

Member

vitaly-krugl commented Jan 31, 2018

Thank you.

lukebakken added a commit that referenced this pull request Apr 12, 2018

Merge pull request #915 from vitaly-krugl/refactor-send-message
Refactor Connection._send_message in terms of _send_frame to avoid  code duplication

(cherry picked from commit 8e9257b)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment