Implement recv_timeout for std::comm *Port implementations #9194
Conversation
|
I'm actually fairly opposed to this change. Polling for synchronization primitives in my opinion is the wrong way to implement timeouts. If you think about the polling intervals, it turns out that polling is wrong 100% of the time. Each time the poller decides to go to sleep, it should not have awoken. When the poller decides that the timeout is done, then it was too late, so every guess by the poller is "wrong" in a sense. It looks like we don't currently have support for accurate timeouts in the libuv loop yet, so I'm ok with these functions existing in some form, but they should be extremely clear that they perform polling and not accurate timeouts. When I write code to have timeouts, I expect the library to have it implemented such that there is no polling and that timeouts occur in a timely fashion. Especially in this implementation you could wait for up to a second waiting for an event to happen when in fact it already happens. In my opinion this extremely undesirable, especially from a core library perspective. Could you explain a bit more why we think that this is the desired strategy to settle on now for timeouts? If this is purely an interim solution (blocked on one easy bug), then I think that it still shouldn't be added, and in the long run if we need some general rearchitecting it seems like we still shouldn't add timeouts in this form. |
|
I've cancelled the build for now, I'd like to resolve some of my concerns first before letting this through bors. |
|
@alexcrichton I agree with you 100% and that is the exact reason why #9195 was created. However, I thought that letting this patch land and then rewrite it after we figure out the state of I'm not opposed to blocking this patch and letting it land after 0.8 release and if by that time we're already able to rewrite it then we can do that instead of letting this one land. I do think we should let the functionality in, asap with the right NOTES / FIXME. |
|
In my opinion, I feel that this an aspect of our primitives which should not be implemented until it's ready to be implemented. I don't think that If we do implement this, it should certainly have |
|
After a discussion on IRC, we've agreed to put this on hold until a few days before 0.8 is released. @flaper87 may have "real timeouts" by then, making this issue moot. If some more work is still required, we'll re-visit this request around that time. |
|
@alexcrichton I rebased the branch and ran tests. Not sure I'll be able to implement the new stuff before 0.8 is released. Sorry about that, last week was very busy. |
| @@ -1 +1 @@ | |||
| Subproject commit c9ffab392a39eb85f2f15ffc8d41e8c4f4397b8e | |||
| Subproject commit 08a3bd96ee6e1d141494f0014ede75d9871114c4 | |||
There was a problem hiding this comment.
I think this may have leaked in at some point.
There was a problem hiding this comment.
Any chance to get this merged for 0.8? As mentioned, I'm already working on
the new version based on Select.
|
@alexcrichton rebased and reverted the llvm commit. |
|
ping @brson |
|
I don't want to implement a recv_timeout function that uses polling. In general we have added to much to the new runtime already before it's stable. We can't keep piling up the debt. |
|
@brson, As mentioned in my previous comments, I agree with this not being optimal, although the idea was to let the API land. I'm already working on the new implementation, hopefully, I'll be able to commit it soon. |
Fix example for `clippy::invalid_regex` Close rust-lang#9194 changelog: previous example doesn't trigger lint
Fixes #3015