Implemented PING fully-featured #409
Conversation
First draft of PingService, which calculates RTT results
There was a problem hiding this comment.
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.
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.
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.
worth at least logging this exception, if not letting bubble up to some caller
There was a problem hiding this comment.
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.
i think ping_limit is much clearer than ping_amount.
want to just use ping_limit everywhere?
There was a problem hiding this comment.
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.
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.
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.
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.
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.
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_amountis passed as argument, it creates an infinite iterator. Every iteration returns the RTT.To-Do
handle_pingHost?