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.

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

Increase delay before checking the uv loop #51003

Closed
wants to merge 1 commit into from

Conversation

timholy
Copy link
Sponsor Member

@timholy timholy commented Aug 22, 2023

This increases the delay slightly before checking whether all I/O
tasks have finished. This may reduce the number of "spurious" warnings
during precompilation.

Fixes #50873

This increases the delay slightly before checking whether all I/O
tasks have finished. This may reduce the number of "spurious" warnings
during precompilation.

Fixes #50873
@timholy
Copy link
Sponsor Member Author

timholy commented Aug 22, 2023

Note that #50914 is still needed to provide more guidance about the "non-spurious" cases. Also, I should acknowledge that 30ms may not be enough either; however, I imagine there's a cost to increasing it even more. Presumably we could do something akin to

if !check_timer_alive()
    return quickly
end

before starting the timer at all (and then with an even longer delay before issuing the first warning), but I don't know the libuv/IO part of Julia at all and don't have time to learn what I'd need to learn to make such a change.

@IanButterworth
Copy link
Sponsor Member

In the spirit of trying to make the UX of the trailing timer UX feel less breaking, as well as this, what if the check had a timeout of say 30s, after which a warning was printed, but the process returned successfully

Warning: A trailing timer did not finish before timeout. The cache may be invalid.

That way warnings are printed but precompilation doesn't hang indefinitely?

@IanButterworth
Copy link
Sponsor Member

If we did that you'd only need one warning before that, (rather than the current repeated one)

FilePathsBase Waiting for background task / IO / timer.
[pid 18736] waiting for IO to finish:
 TYPE[FD/PID]       @UV_HANDLE_T->DATA
 timer              @0x600002cbed00->0x10da90ee0

Warning: A trailing timer did not finish before timeout. The cache may be invalid.

@brenhinkeller brenhinkeller added the domain:io Involving the I/O subsystem: libuv, read, write, etc. label Aug 23, 2023
@IanButterworth
Copy link
Sponsor Member

IanButterworth commented Aug 27, 2023

More reports of confusion here, where 3/4 package outputs look like they might be fixed by increasing this delay? (MKL_jll is clearly real as it's downloading)
https://discourse.julialang.org/t/waiting-for-io-to-finish-1-10-beta2/103240

@timholy
Copy link
Sponsor Member Author

timholy commented Aug 27, 2023

Also https://discourse.julialang.org/t/waiting-for-io-to-finish-1-10-beta2/103240

This has been brought up often enough that I'm tagging it as a milestone for 1.10.

@timholy timholy added this to the 1.10 milestone Aug 27, 2023
@vtjnash
Copy link
Sponsor Member

vtjnash commented Aug 31, 2023

MKL_jll is not real, that is a Downloads.jl issue

@timholy
Copy link
Sponsor Member Author

timholy commented Sep 12, 2023

@vtjnash, what's your thought here? Is this a package bug (the package is responsible for wait(task) to ensure it finishes)? Or does Julia need to be more patient, and if so is this the right way to do it?

@vtjnash
Copy link
Sponsor Member

vtjnash commented Sep 13, 2023

Our current thinking is we can ask Julia for more information about the handle and try to get something extra from that to print

@timholy
Copy link
Sponsor Member Author

timholy commented Sep 13, 2023

That would be really, really nice. Some way to trace Tasks back to their creation point would be a fantastic step forward. Perhaps one wants @Task and have it store the @__FILE__ and @__LINE__? Though perhaps type-inference could manually insert that (a bit messy though...)

@PallHaraldsson
Copy link
Contributor

PallHaraldsson commented Oct 11, 2023

package outputs look like they might be fixed by increasing this delay?

Since a delay of 30 ms (proposed here, and also current 10) is just arbitrary, but seemingly not too bad, should this just be merged? This is "mitigation (for a warning), so seems harmless, while not a "fix"? Is there a better fix, or bad affect with merging this, hiding some real problem?

@IanButterworth
Copy link
Sponsor Member

A couple of sources of spurious triggers for these messages have now been fixed (on master and next 1.10) so I think we could close this and see if the issue returns.

As for the deadlock where the timer message loops, that seems to be #51662

@vtjnash vtjnash deleted the teh/io_precompile_timer branch October 11, 2023 13:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
domain:io Involving the I/O subsystem: libuv, read, write, etc.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Warnings during precompilation on v1.10-beta1: waiting for IO to finish
6 participants