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

mininode: add an optimistic write and disable nagle #11323

Merged
merged 1 commit into from Sep 18, 2017
Merged

mininode: add an optimistic write and disable nagle #11323

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
10 changes: 9 additions & 1 deletion test/functional/test_framework/mininode.py
View file
Open in desktop
Expand Up @@ -1654,6 +1654,7 @@ def __init__(self, dstaddr, dstport, rpc, callback, net="regtest", services=NODE
self.dstaddr = dstaddr
self.dstport = dstport
self.create_socket(socket.AF_INET, socket.SOCK_STREAM)
self.socket.setsockopt(socket.IPPROTO_TCP, socket.TCP_NODELAY, 1)
self.sendbuf = b""
self.recvbuf = b""
self.ver_send = 209
Expand Down Expand Up @@ -1792,7 +1793,14 @@ def send_message(self, message, pushbuf=False):
tmsg += h[:4]
tmsg += data
with mininode_lock:
self.sendbuf += tmsg
if (len(self.sendbuf) == 0 and not pushbuf):
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: if not self.sendbuf and not pushbuf:. From PEP8: "For sequences, (strings, lists, tuples), use the fact that empty sequences are false."

Copy link
Member

Choose a reason for hiding this comment

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

No need to fix this "style nit". len(some_list)==0 is stricter than just casting any object/primitive type to boolean. Also it is more clear to read, imo.

Copy link
Member

Choose a reason for hiding this comment

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

@MarcoFalke I believe that for values of the list type if len(some_list)==0 is actually identical to if not some_list, and in general I'd argue for following things like PEP8.

Copy link
Contributor

Choose a reason for hiding this comment

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

ACK with or without this nit change. I have a slight preference for being pythonic and following PEP8. Leaving the longer form (if (len(self.sendbuf) ==0 would catch any bugs where someone set self.sendbuf to something other than a list, but I don't think that's a good justification.

Copy link
Contributor

Choose a reason for hiding this comment

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

Either way I think the brackets should at least be removed

Copy link
Member

Choose a reason for hiding this comment

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

@sipa, it is identical. Though, in the case that some_list is False or None or 0 or anything else that casts to False, len(some_list) will fail appropriately. I don't think it is a downside to fail in the test framework when a parameter has the wrong type.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think to be hyper pedantic, if len(self.sendbuf) == 0 is equivalent to:

if not hasattr(self.sendbuf, '__len__'):
    raise TypeError
else if not self.sendbuf:
    # do stuff

ie, it basically has an implicit assert that the type of self.sendbuf has a length property.

In general we don't litter the Python code with type checks, even though it's not a type safe language and someone could call into functions with the wrong typed parameters.

Copy link
Member

Choose a reason for hiding this comment

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

👍 on @jimpo and @meshcollider suggestions.

try:
sent = self.send(tmsg)
self.sendbuf = tmsg[sent:]
except BlockingIOError:
self.sendbuf = tmsg
else:
self.sendbuf += tmsg
self.last_sent = time.time()

def got_message(self, message):
Expand Down