-
Notifications
You must be signed in to change notification settings - Fork 101
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
Implemented PING fully-featured #409
base: main
Are you sure you want to change the base?
Conversation
First draft of PingService, which calculates RTT results
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks for this!
looks pretty good, i left some questions/comments...
this could just be me but the Union[int,float]
type seems a little strange in the interface to ping_loop
. Another route to consider is something more like Optional[int]
with a default value of None
. if PingIterator
is not given an upper bound then it basically just becomes yield
inside a while True:
loop inside the __aiter__
.
|
||
async def _ping(stream: INetStream) -> int: | ||
ping_bytes = secrets.token_bytes(PING_LENGTH) | ||
before = int(time.time() * 10 ** 6) # convert float of seconds to int miliseconds |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what do you think about just keeping the native time.time()
value as a float
for as long as possible (possibly just letting the consumer of the rtt convert to int
if they want...)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
First of all thank you for your comments. I learn a lot from them!
I think it is a good idea to keep it as float. Do you think it is better to leave it also as native seconds? I think I prefer to return it as a miliseconds float
libp2p/host/ping.py
Outdated
try: | ||
await stream.close() | ||
except Exception: | ||
pass |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
worth at least logging this exception, if not letting bubble up to some caller
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you are right, it is better to let bubble up to the caller
libp2p/host/ping.py
Outdated
self, peer_id: PeerID, ping_amount: Union[int, float] = math.inf | ||
) -> "PingIterator": | ||
stream = await self._host.new_stream(peer_id, (ID,)) | ||
ping_iterator = PingIterator(stream, ping_amount) | ||
return ping_iterator | ||
|
||
|
||
class PingIterator: | ||
def __init__(self, stream: INetStream, ping_amount: Union[int, float]): | ||
self._stream = stream | ||
self._ping_limit = ping_amount |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i think ping_limit
is much clearer than ping_amount
.
want to just use ping_limit
everywhere?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like it! I'll change it to ping_limit
async def handle_ping(stream: INetStream) -> None: | ||
"""``handle_ping`` responds to incoming ping requests until one side errors | ||
or closes the ``stream``.""" | ||
peer_id = stream.muxed_conn.peer_id | ||
|
||
while True: | ||
try: | ||
should_continue = await _handle_ping(stream, peer_id) | ||
if not should_continue: | ||
return | ||
except Exception: | ||
await stream.reset() | ||
return |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
any particular reason to move this chunk of code up here?
just curious.... at first pass on this review i figured you had changed some of the logic but it seems to be the same(!)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh I'm sorry, I think this happened because at first I moved it inside PingService
as a method. But at the end I left it out, and I think I moved it accidentally
async def ping_loop( | ||
self, peer_id: PeerID, ping_amount: Union[int, float] = math.inf | ||
) -> "PingIterator": | ||
stream = await self._host.new_stream(peer_id, (ID,)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
would be helpful to leave a docstring describing why we have ping
and ping_loop
.
it turns out that ping
is just ping_loop
specialized to ping_amount == 1
. which also suggest we may be able to simplify the implementation here...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay, it is a good idea to write a docstring!
The reason why I thought of separating into two methods is because ping_loop
is a way to use it as an intuitive for loop
. However, it is not so straightforward, you must generate the iterator with ping_loop
and then construct a for loop
. So I created the second method ping
for when you just want a quick, simple and one-line ping (maybe for a rapid keep-alive, or something else).
If am open to discussion whether it really is worth it or useful or not :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also the loop is useful in case you want to add some more functionality between each ping. (or maybe it is not a good feature?)
However, I have been thinking and maybe it is a good idea to add a second argument to the ping(peer_id)
, which specifies the amount of pings and return a list of RTTs
. Something like rtts = ping_service.ping(peer_id, amount=5)
.
@ralexstokes I have updated the |
First draft of PingService, which executes pings and implements RTT calculation feature.
What was wrong?
There was no PingService that implemented the RTT feature, nor an easy way for creating a Ping requests Loop.
Issue #344
How was it fixed?
Created a class Called PingService, which implements two methods:
ping(peer_id)
: method for executing a ping. It opens a stream, pings it, closes the stream, and returns the RTT.ping_loop(peer_id, ping_amount)
: method that returns a Ping iterator. If noping_amount
is passed as argument, it creates an infinite iterator. Every iteration returns the RTT.To-Do
handle_ping
Host
?