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

mktemp is using a directory that doesn't yet exist? #8

Closed
mbauman opened this issue Mar 19, 2021 · 11 comments
Closed

mktemp is using a directory that doesn't yet exist? #8

mbauman opened this issue Mar 19, 2021 · 11 comments

Comments

@mbauman
Copy link
Member

mbauman commented Mar 19, 2021

I have a blob. I try to open it. I get:

julia> open(io->CSV.read(io, DataFrame), IO, blob)
ERROR: SystemError: mktemp: No such file or directory

I redefined mktemp to show me what it's trying to do:

julia> @eval Base.Filesystem function mktemp(parent::AbstractString=tempdir(); cleanup::Bool=true)
           @show parent
           b = joinpath(parent, temp_prefix * "XXXXXX")
           @show b
           p = ccall(:mkstemp, Int32, (Cstring,), b) # modifies b
           systemerror(:mktemp, p == -1)
           cleanup && temp_cleanup_later(b)
           return (b, fdio(p, true))
       end
mktemp (generic function with 4 methods)

julia> open(io->CSV.read(io, DataFrame), IO, blob)
parent = "/tmp/jl_Oh98Jz"
b = "/tmp/jl_Oh98Jz/jl_XXXXXX"
ERROR: SystemError: mktemp: No such file or directory

I make that directory:

shell> mkdir /tmp/jl_Oh98Jz

And now it works:

julia> open(io->CSV.read(io, DataFrame), IO, blob)
parent = "/tmp/jl_Oh98Jz"
b = "/tmp/jl_Oh98Jz/jl_XXXXXX"
1105438×6 DataFrame

(This is in VS Code on JuliaHub, using the workaround to get a Data.toml from JuliaHub)

@mbauman
Copy link
Member Author

mbauman commented Mar 19, 2021

Ah, and I see it in submitted jobs on JuliaHub, too.

Code:

using DataSets, CSV, DataFrames
blob = open(Blob, dataset("us_counties"))
df = open(io->CSV.read(io, DataFrame), IO, blob)
@show length(df)
ENV["OUTPUTS"] = """{
    "len":$(length(df))
}"""

Error:

LoadError: LoadError: SystemError: mktemp: No such file or directory Stacktrace: [1] systemerror(::Symbol, ::Int32; extrainfo::Nothing) at ./error.jl:168 [2] #systemerror#48 at ./error.jl:167 [inlined] [3] systemerror at ./error.jl:167 [inlined] [4] #mktemp#18 at ./file.jl:562 [inlined] [5] find_in_cache(::JHubDataRepos.DataRepoCache, ::String) at /home/jrun/projects/default/JHubDataRepos/src/data_cache.jl:119 ...

@c42f
Copy link
Contributor

c42f commented Mar 24, 2021

I think the problem here is that the blob cache isn't persistent - it assumes you use it in scoped form:

open(Blob, dataset("foo")) do blob
    # do something with `blob`
end
# backing cache for `blob` is deleted at this point

I've been finding the scoped-form-vs-not is a bit of a design catch-22:

  • The nesting of the scoped form gets out of control easily and it's really not that nice to look at
  • But it's the only way to guarantee resource cleanup (not just of downloaded data, but other programmatic resources like async downloader tasks)

I guess I should figure out a way to disable the non-scoped form open(Blob, dataset("foo")). It works for local storage, but not for storage drivers which do resource cleanup like the JuliaHub storage driver.

@pfitzseb
Copy link
Member

Relying on the OS to clean up /tmp seems like it might be good enough, no?

@StefanKarpinski
Copy link
Member

StefanKarpinski commented Apr 28, 2021

But it's the only way to guarantee resource cleanup (not just of downloaded data, but other programmatic resources like async downloader tasks)

Perhaps I'm missing something, but finalizers are the typical way to do this. Do finalizers not work for this? The general pattern is: let people use the block form for deterministic scoped cleanup but use finalizers to allow unscoped cleanup whenever GC determines that something is no longer reachable. In this case there's even a Blob object that the finalizer can be attached to.

@StefanKarpinski
Copy link
Member

@c42f, does that make sense? Is there any issue with using finalizers for this?

@c42f
Copy link
Contributor

c42f commented Apr 30, 2021

Yeah sorry, I was thinking about this yesterday and trying to write down my thoughts. I do try to avoid finalizers if at all possible so I'd kind of forgotten about them for this use case. But they're really the only tool we have at hand here.

Supporting both finalizers as well as the scoped form does change the API which data backends will need to implement. The job of a data backend (in, for example, JuliaHubDataRepos) is currently to implement a single function

connect_the_backend(user_func, config, dataset)

which initializes the resources needed by the backend, runs user_func with them, then tears everything down. (Here connect_the_backend is not a generic function defined within DataSets - it's just any callable which is looked up based on the backend driver name.)

If we want open to return an open resource (with finalizer attached) but also have the scoped form, we probably need to scrap this as the low level API, and instead go with open and close-like verbs. Those define a state machine which we can build both the finalizer-based version and the scoped version on top of.

(As an aside, there's also the a transformation which can turn any scoped resource handling into a state machine using the @async described roughly in JuliaLang/julia#33248 (comment). But this is kind of a mess.)

@StefanKarpinski
Copy link
Member

Why does everybody hate finalizers? They're very useful.

@c42f
Copy link
Contributor

c42f commented Apr 30, 2021

Because they run on whatever random task happens to be running the GC so there's no good way to deal with failure inside a finalizer. Any exceptions get swallowed because there's no context in which to report them. Task switching is disallowed, so even logging the error is more difficult than it should be.

All of that makes finalizers a poor place to deal with operations that can fail so I try to only use them as a last resort.

@c42f
Copy link
Contributor

c42f commented May 13, 2021

I have a potential fix for this at #12.

Yes, I did feel the need to implement a whole new resource management package just to fix this issue 😅

@c42f
Copy link
Contributor

c42f commented Jun 4, 2021

Should be fixed in #12 and release 0.2.3

@c42f
Copy link
Contributor

c42f commented Jun 4, 2021

@c42f c42f closed this as completed Jun 4, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants