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

test/file: try to fix freezing of test_22566 #23001

Merged
merged 1 commit into from
Aug 29, 2017

Conversation

iblislin
Copy link
Member

or any suggestion to test open_cloexec in src/support/ios.c ?

@vtjnash
Copy link
Member

vtjnash commented Jul 28, 2017

What does this do? A simple blocking read/write of a byte would be preferable to a timed sleep.

@iblislin
Copy link
Member Author

iblislin commented Jul 28, 2017

@vtjnash recent CI build froze quite frequently after my PR #22566 merged.
I can reproduce the freezing on local via runing gmake test 2~5 times.
I attached my GDB to check the bt of workers, one of the workers stucks at the reader's open (r = open(fn, "r")).
So I guess that there is race condiction between the writer's open and reader's, but still no idea about the root cause.

@iblislin
Copy link
Member Author

A simple blocking read/write of a byte would be preferable to a timed sleep.

I will try it tmr. :)

@ararslan ararslan added the test This change adds or pertains to unit tests label Jul 28, 2017
@iblislin
Copy link
Member Author

+++ b/test/file.jl
@@ -1141,11 +1141,12 @@ if !Sys.iswindows()
         fn = tempname()
         run(`mkfifo $fn`)

-        script = "x = open(\"$fn\", \"w\"); sleep(0.1); close(x); quit()"
+        script = "x = open(\"$fn\", \"w\"); write(x, 0x42); flush(x); close(x); quit()"
         cmd = `$(Base.julia_cmd()) --startup-file=no -e $script`
         open(pipeline(cmd, stderr=STDERR))

         r = open(fn, "r")
+        @test read(r, Int64) == 66
         close(r)

         rm(fn)

got this

file: Error During Test
  Test threw an exception of type EOFError
  Expression: read(r, Int64) == 66
  EOFError: read end of file
  Stacktrace:
   [1] read at ./iostream.jl:202 [inlined]
   [2] test_22566() at /usr/home/iblis/git/julia/test/file.jl:1149
   [3] macro expansion at /usr/home/iblis/git/julia/test/file.jl:1157 [inlined]
   [4] anonymous at ./<missing>:?
   [5] include_relative(::Module, ::String) at ./loading.jl:464
   [6] include at ./sysimg.jl:14 [inlined]
   [7] include(::String) at /usr/home/iblis/git/julia/test/testdefs.jl:11
   [8] macro expansion at /usr/home/iblis/git/julia/test/testdefs.jl:18 [inlined]
   [9] macro expansion at ./test.jl:943 [inlined]
   [10] macro expansion at ./util.jl:379 [inlined]
   [11] macro expansion at /usr/home/iblis/git/julia/test/testdefs.jl:17 [inlined]
   [12] anonymous at ./<missing>:?

or I'm okay with getting rid of this test case..

@vtjnash
Copy link
Member

vtjnash commented Jul 29, 2017

write(x, 0x42);

You're only writing a byte, but attempting to read an Int64

test/file.jl Outdated
@@ -1141,7 +1141,7 @@ if !Sys.iswindows()
fn = tempname()
run(`mkfifo $fn`)

script = "x = open(\"$fn\", \"w\"); close(x)"
script = "x = open(\"$fn\", \"w\"); sleep(0.1); close(x); quit()"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

while we're changing this test, can you replace \"$fn\" with $(repr(fn)) for me too? (that'll make it robust against other random meta-characters happening to to get inject into temp path)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🆗

- wrap the long single scripts

- using `$(repr(fn))` for inserting temp file name

- use STDIN to synchronize fifo open calls for each end
@vtjnash vtjnash merged commit 90c91e3 into JuliaLang:master Aug 29, 2017
@iblislin iblislin deleted the fix-freeze branch August 30, 2017 06:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
test This change adds or pertains to unit tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants