-
-
Notifications
You must be signed in to change notification settings - Fork 5.5k
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
pollfd fix, round 2 #12237
pollfd fix, round 2 #12237
Conversation
this eliminates the callback interface to the filesystem poll functions, to be consistent with the changes to the rest of base additionally, this exposes the full libuv API for watch_file and poll_file, rather than returning inconsistent return values from these two functions this fixes FDWatcher to ensure that it is unique for any given fd, (to avoid a libuv assertion). it must be manually closed before closing fd, or the applications behavior is undefined.
cross-ref: this will resolve some of #10293 |
this closes #7840 (when merged) |
@@ -595,6 +597,7 @@ function _uv_hook_close(t::Timer) | |||
unpreserve_handle(t) | |||
disassociate_julia_struct(t) | |||
t.handle = C_NULL | |||
notify(t.cond) |
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.
this fix is the cause of the warning in the repl.jl
test: the Timer callback on line 100 of test/repl.jl is supposed to accept an argument t
Awesome, this is a relief! I believe the only piece of #10293 left is SingleAsyncWork? |
end | ||
|
||
@unix_only _get_osfhandle(fd::RawFD) = fd | ||
@windows_only _get_osfhandle(fd::RawFD) = WindowsRawSocket(ccall(:_get_osfhandle,Ptr{Void},(Cint,),fd.fd)) |
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.
this'll need to be qualified with Libc.
now anywhere it's used, like base/stream.jl
and base/mmap.jl
@@ -1,52 +1,90 @@ | |||
# This file is a part of Julia. License is MIT: http://julialang.org/license | |||
|
|||
@unix_only begin |
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.
why did you remove this? msvcrt has _pipe
but it has extra arguments: https://msdn.microsoft.com/en-us/library/aa298531(v=vs.60).aspx
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.
the eventual goal is to get this test running on windows too
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.
not a bad goal, but will need to account for different spellings/api of posix functions - unless libuv has wrappers for all of these, which I think is one of the things we've been meaning to do upstream?
also works-around an issue where libuv timers could return too early on windows by several milliseconds this provides a general-purpose socketpair implementation on windows, for use in the tests and otherwise (although uv_pipe_link is recommended for any case where you don't explicitly need a WSA2 SOCKET)
…ows] Libc module (also use them to provide better errors in the tests) these belong in Libc since the are the windows counterparts to errno/strerror which are documented and exported in Libc
bump. this passes CI everywhere now... |
Monitor a file for changes by polling every `interval_seconds` seconds for `seconds` seconds. A return value of true indicates | ||
the file changed, a return value of false indicates a timeout. | ||
Monitor a file for changes by polling every ``interval_seconds`` seconds until a change occurs or ``timeout_s`` seconds have elapsed. | ||
The `timeout_s` should be a long interval; the default is 5.007 seconds. |
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.
Should say interval instead of timeout_s.
Marking this as 0.4 - assuming this is is done and long standing, and we should get it in. |
bump. i'll merge this later today. |
Awesome. 👍 |
travis failure here https://gist.github.com/tkelman/96b21860b35454115373, are we back to timings that are too picky for busy low-memory VM's? |
OTOH, in this case it does seem like it caught an actual bug. The poll call should not have returned before |
Another failure: https://travis-ci.org/JuliaLang/julia/jobs/73427729
|
another pollfd failure on travis https://s3.amazonaws.com/archive.travis-ci.org/jobs/75113270/log.txt and https://s3.amazonaws.com/archive.travis-ci.org/jobs/75295368/log.txt |
Another pollfd failure on AppVeyor: |
i'm not really sure what that test is doing in the pollfd.jl file, since it is testing the functionality of one-shot Timers, not any of this code @JeffBezanson @Keno is 0.2 sec sometimes not enough for a 0.01 sec timer to trigger? (added in 7f4c2de) |
same |
latest travis failure on #12463 (https://travis-ci.org/JuliaLang/julia/builds/76174785) was also due to Jeff's new test for Timers there |
this is a rewrite of the poll_fd, poll_file, and watch_file APIs to more directly expose the libuv API and eliminate the callback legacy interface. since
FDWatcher
objects are now refcounted, this also solves #4401.this replaces #7275
ping: @Keno