reliable flush/close on streams, and avoid zombies #17522

vtjnash · 2016-07-21T01:03:17Z

this makes it so that our close() function waits for the shutdown to finish before returning (#14434)

@Keno for review of libuv changes

tkelman · 2016-07-21T01:05:31Z

deps/libuv.version

-LIBUV_BRANCH=julia-uv1.9.0
-LIBUV_SHA1=ecbd6eddfac4940ab8db57c73166a7378563ebd3
+LIBUV_BRANCH=jn/pid-zero
+LIBUV_SHA1=13c00a4268a16d5a12f3cd15b3a40c683e575dfc


before merging this should go on the branch, and checksums should be updated

vtjnash · 2016-07-22T02:57:31Z

deps/libuv.version

-LIBUV_BRANCH=julia-uv1.9.0
-LIBUV_SHA1=ecbd6eddfac4940ab8db57c73166a7378563ebd3
+LIBUV_BRANCH=jn/pid-zero
+LIBUV_SHA1=cb6d0f875a5b8ca30cba45c0c1ef7442c87c1e68


before merging this should go on the branch, and checksums should be updated

you could also mark this as wip

it's not WIP, it's simply waiting for review across two repos

work remains to be done, sounds "in progress" to me

that's not what the WIP tag is intended to convey

that's exactly what the WIP tag is intended to convey

The convention is to prefix the pull request title with "WIP:" for Work In Progress, or "RFC:" for Request for Comments when work is completed and ready for merging. This will prevent accidental merging of work that is in progress.

the work is done and ready for merging. this just requires slightly more manual work than the usual Github button. however, we don't consider the merging "work" in the WIP tag.

(aside: I find it strange when there is both a reply and reaction on a comment. aren't they conveying redundant information?)

no, this PR is not ready for merging. in what sense is "requires slightly more manual work" not precisely a form of "work" that should not be accidentally merged in its current state?

In the same way that #17579 was not WIP, even though it was not merged in its PR state.

that was a squash and the diff as pushed to master was identical to the pr state

vtjnash · 2016-07-24T02:23:49Z

bump @Keno (or maybe @StefanKarpinski?). CI failure were unrelated

Keno · 2016-07-25T21:11:44Z

base/stream.jl

+            ccall(:jl_close_uv, Void, (Ptr{Void},), stream.handle)
+            stream.status = StatusClosing
+        end
+        if uv_handle_data(stream) != C_NULL


What situation is this checking for and why was it not needed previously?

this is because of the changed order in uvfinalize

Keno · 2016-07-25T21:15:07Z

So the problem is that libuv does not reap zombies if we close the handle before the process has exited?

vtjnash · 2016-07-26T00:49:17Z

So the problem is that libuv does not reap zombies if we close the handle before the process has exited?

right. if we close the libuv handle, then libuv has no way to track when the process dies and reap the zombie. so we need to instead ensure the libuv handle is valid for as long as its pid is non-zero.

tkelman · 2016-07-26T09:11:28Z

Not that our relationship with libuv upstream is all that great right now, but does this fix a bug that they might care about?

vtjnash · 2016-07-26T15:04:05Z

Probably not, nodejs don't expose a close api

tkelman · 2016-07-29T05:15:13Z

deps/libuv.version

@@ -1,2 +1,2 @@
 LIBUV_BRANCH=julia-uv1.9.0
-LIBUV_SHA1=ecbd6eddfac4940ab8db57c73166a7378563ebd3
+LIBUV_SHA1=cb6d0f875a5b8ca30cba45c0c1ef7442c87c1e68


needs checksums updated

and delete the old

fix #14434

while investigating #14434, I noticed that the test code there is creating zombies to fix that, we need to avoid calling uv_close in the finalizer until after the process has exited (or during atexit)

tkelman · 2016-08-04T07:03:16Z

This is causing spawn test failures https://ci.appveyor.com/project/JuliaLang/julia/build/1.0.5013/job/ey1xj2tloe8x47lt. Can reproduce locally.

tkelman · 2016-08-04T07:15:48Z

reverting on release-0.5 for rc1

…ose" This reverts commit 08ae28a, reversing changes made to 1eeb773. This is causing spawn test failures on Windows.

vtjnash · 2016-08-04T12:26:10Z

That test is stronger than essential, so it may need another Base.process_events call on Windows to fully work (hard to say, since this is testing the kernel as much as it is testing Julia)

tkelman · 2016-08-04T12:27:43Z

I'll try that.

tkelman · 2016-08-04T12:47:49Z

If you meant

diff --git a/test/spawn.jl b/test/spawn.jl
index 0040630..0cc4184 100644
--- a/test/spawn.jl
+++ b/test/spawn.jl
@@ -287,7 +287,11 @@ let out = Pipe(), echo = `$exename --startup-file=no -e 'print(STDOUT, " 1\t", r
         @test_throws ArgumentError write(out, "now closed error")
         @test isreadable(out)
         @test !iswritable(out)
-        is_windows() && Base.process_events(false) # should be enough steps to fully propagate EOF now
+        if is_windows()
+            # should be enough steps to fully propagate EOF now
+            Base.process_events(false)
+            Base.process_events(false)
+        end
         @test !isopen(out)
     end
     wait(ready) # wait for writer task to be ready before using `out`

then no that didn't help

vtjnash · 2016-08-04T13:20:51Z

Maybe sleep(0.04)? I think that's the documented maximum time for this to propagate. But how about we just go with is_windows() && Base.wait_close(out) # WINNT kernel does not provide a fast mechanism for async propagation of EOF for a blocking stream, so just wait for it to catch up. This shouldn't take much more than 32ms.

introduced by #17522

tkelman · 2016-08-09T00:18:12Z

This caused ZMQ.jl's tests to freeze on Windows. I'm even more glad I reverted this, and won't be backporting it until that gets addressed.

vtjnash · 2016-08-09T01:03:40Z

Is this supposed be some sort of guessing game? Usually bug reports should contain a little more context and a little less snark

But if I have to guess, I would point to the bug in the following test:
https://github.com/JuliaLang/ZMQ.jl/blob/master/test/runtests.jl#L90
which is supposed to freeze (it didn't previously, but that's why #14434 existed). Although on some kernels it is often allowed to pass due to the small amount of data involved.

tkelman · 2016-08-09T01:13:06Z

Have no context yet, have identified the commit that introduced the problem but not yet identified which line was freezing. Sorry I'm extremely frustrated that we're trying to put out a release and last-minute bug fixes have been causing as many issues as they fix.

…flush-close"" This reverts commit 58a14f3.

introduced by #17522 (cherry picked from commit a3f288c) ref #17815

introduced by JuliaLang#17522

tkelman reviewed Jul 21, 2016
View reviewed changes

tkelman added the io Involving the I/O subsystem: libuv, read, write, etc. label Jul 21, 2016

vtjnash force-pushed the jn/reliable-flush-close branch from 7e9a7dc to 1be9e76 Compare July 21, 2016 17:11

vtjnash reviewed Jul 22, 2016
View reviewed changes

Keno reviewed Jul 25, 2016
View reviewed changes

vtjnash force-pushed the jn/reliable-flush-close branch from 1be9e76 to 4ba3790 Compare July 29, 2016 03:03

tkelman reviewed Jul 29, 2016
View reviewed changes

vtjnash force-pushed the jn/reliable-flush-close branch 3 times, most recently from 389e2eb to 8822ccb Compare August 1, 2016 00:42

vtjnash added 2 commits July 31, 2016 23:16

wait for stream to finish flush before returning from close

c4b1ed6

fix #14434

avoid leaving process zombies

67efdcf

while investigating #14434, I noticed that the test code there is creating zombies to fix that, we need to avoid calling uv_close in the finalizer until after the process has exited (or during atexit)

vtjnash force-pushed the jn/reliable-flush-close branch from 8822ccb to 67efdcf Compare August 1, 2016 03:17

vtjnash merged commit 08ae28a into master Aug 4, 2016

vtjnash deleted the jn/reliable-flush-close branch August 4, 2016 03:38

tkelman added a commit that referenced this pull request Aug 4, 2016

Revert "Merge pull request #17522 from JuliaLang/jn/reliable-flush-cl…

58a14f3

…ose" This reverts commit 08ae28a, reversing changes made to 1eeb773. This is causing spawn test failures on Windows.

tkelman mentioned this pull request Aug 4, 2016

Checklist of steps towards 0.5 release [candidates] #17418

Closed

16 tasks

vtjnash added the backport pending 0.5 label Aug 4, 2016

tkelman added a commit that referenced this pull request Aug 4, 2016

Fix #17805, spawn test failure on Windows

a3f288c

introduced by #17522

tkelman mentioned this pull request Aug 4, 2016

Fix #17805, spawn test failure on Windows #17815

Merged

tkelman added a commit that referenced this pull request Aug 11, 2016

Revert "Revert "Merge pull request #17522 from JuliaLang/jn/reliable-…

c7cc66f

…flush-close"" This reverts commit 58a14f3.

tkelman added a commit that referenced this pull request Aug 11, 2016

Fix #17805, spawn test failure on Windows

7306033

introduced by #17522 (cherry picked from commit a3f288c) ref #17815

tkelman removed the backport pending 0.5 label Aug 12, 2016

ssfrr mentioned this pull request Aug 28, 2016

can't redirect STDOUT and STDERR to DevNull #18268

Closed

mfasi pushed a commit to mfasi/julia that referenced this pull request Sep 5, 2016

Fix JuliaLang#17805, spawn test failure on Windows

b954f3a

introduced by JuliaLang#17522

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

reliable flush/close on streams, and avoid zombies #17522

reliable flush/close on streams, and avoid zombies #17522

vtjnash commented Jul 21, 2016

tkelman Jul 21, 2016

vtjnash Jul 22, 2016

tkelman Jul 22, 2016

vtjnash Jul 22, 2016

tkelman Jul 22, 2016

vtjnash Jul 24, 2016

tkelman Jul 24, 2016

vtjnash Jul 24, 2016

tkelman Jul 24, 2016

vtjnash Jul 25, 2016

tkelman Jul 26, 2016

vtjnash commented Jul 24, 2016

Keno Jul 25, 2016

vtjnash Jul 26, 2016

Keno commented Jul 25, 2016

vtjnash commented Jul 26, 2016

tkelman commented Jul 26, 2016

vtjnash commented Jul 26, 2016

tkelman Jul 29, 2016

tkelman Jul 30, 2016

tkelman commented Aug 4, 2016

tkelman commented Aug 4, 2016

vtjnash commented Aug 4, 2016

tkelman commented Aug 4, 2016

tkelman commented Aug 4, 2016

vtjnash commented Aug 4, 2016

tkelman commented Aug 9, 2016

vtjnash commented Aug 9, 2016

tkelman commented Aug 9, 2016 •

edited

Loading

reliable flush/close on streams, and avoid zombies #17522

reliable flush/close on streams, and avoid zombies #17522

Conversation

vtjnash commented Jul 21, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vtjnash commented Jul 24, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Keno commented Jul 25, 2016

vtjnash commented Jul 26, 2016

tkelman commented Jul 26, 2016

vtjnash commented Jul 26, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tkelman commented Aug 4, 2016

tkelman commented Aug 4, 2016

vtjnash commented Aug 4, 2016

tkelman commented Aug 4, 2016

tkelman commented Aug 4, 2016

vtjnash commented Aug 4, 2016

tkelman commented Aug 9, 2016

vtjnash commented Aug 9, 2016

tkelman commented Aug 9, 2016 • edited Loading

tkelman commented Aug 9, 2016 •

edited

Loading