Cannot close file opened async #4334

Closed
andreaferretti opened this Issue Jun 14, 2016 · 17 comments

Projects

None yet

3 participants

@andreaferretti
Contributor

It seems that closing a file opened with openAsync fails. Saving this example

import asyncdispatch, asyncfile

proc test(path: string) {.async.} =
  let f = openAsync(path)
  try:
    close(f)
  except:
    let e = getCurrentException()
    echo e.msg

waitFor test("test.nim")

as test.nim and then running nim c -r test reports Operation not permitted.

All this running under Ubuntu 15.04 using latest Nim devel (commit 5e86465)

@dom96 dom96 added the Stdlib label Jun 14, 2016
@dom96
Member
dom96 commented Jun 14, 2016 edited

Could be due to this fb4ff86 ?

@cheatfate any ideas?

@andreaferretti
Contributor

Dunno. But are there not tests in place for asyncfile?

@cheatfate
Contributor

@dom96, mostly because of this:
selectors.nim:135

  proc unregister*(s: var Selector, fd: SocketHandle) =
    if epoll_ctl(s.epollFD, EPOLL_CTL_DEL, fd, nil) != 0:
      let err = osLastError()
      if err.cint notin {ENOENT, EBADF}:
        # TODO: Why do we sometimes get an EBADF? Is this normal?
        raiseOSError(err)
    s.fds.del(fd)

But to avoid this bug i think it must be:

  proc unregister*(s: var Selector, fd: SocketHandle) =
    if s.fds[fd].events != {}:
      if epoll_ctl(s.epollFD, EPOLL_CTL_DEL, fd, nil) != 0:
        let err = osLastError()
        if err.cint notin {ENOENT, EBADF}:
          # TODO: Why do we sometimes get an EBADF? Is this normal?
          raiseOSError(err)
    s.fds.del(fd)

Because there no operations was made on this file, descriptor was not registered in epoll, and when you tried to delete it from epoll... error happens...

@andreaferretti
Contributor

I do not understand fully, but if you add the line

let contents = await f.readAll

the exception is still raised.

@cheatfate
Contributor

@andreaferretti there 2 ways :)

  1. i will explain you (hard way).
  2. you will patch your selectors.nim (easy way).

And some easy explanations. readAll() uses read() which after one successful call makes call to selectors.nim:update() which call epoll_ctl(s.epollFD, EPOLL_CTL_DEL, fd, nil) because there no callbacks registered. So when you exit from await f.readAll(), your descriptor was already unregistered from epoll.

@cheatfate
Contributor

@andreaferretti but if you really want, i can make more wide explanation.

@cheatfate
Contributor

@andreaferretti is patch works for you?

@andreaferretti
Contributor

Let me try

@andreaferretti
Contributor

@cheatfate Yes, it seems to work fine with the modification you proposed

@cheatfate
Contributor

@andreaferretti try not use readAll() because its current implementation is too expensive, it makes reads in cycle of:

  1. register in epoll
  2. epoll_wait()
  3. read()
  4. unregister from epoll()

but must be:

  1. epoll_wait()
  2. read()
@dom96 dom96 pushed a commit that closed this issue Jun 14, 2016
@cheatfate cheatfate Resolve #4334 efb4d97
@dom96 dom96 closed this in efb4d97 Jun 14, 2016
@andreaferretti
Contributor

So, what do you suggest instead to read the whole content of a file?

@dom96
Member
dom96 commented Jun 14, 2016

The bigger (unfortunately) problem with asyncfile is that it's not always async (I think on Linux for example). But depending on your use cases it's "good enough".

I guess the readAll proc could retrieve the file size and then call read(fileSize) to read everything at once. That's the only way I see of making it more efficient.

@andreaferretti
Contributor

My use case is just serving static files in Rosencrantz. I am planning to implement streaming files using chunked encoding, but for now I just support serving the whole file at once

@dom96
Member
dom96 commented Jun 14, 2016

Then it's fine. It will work and we can fix performance problems later without changes to your code.

@cheatfate
Contributor

If you want to be effective try to use sendfile()

@cheatfate
Contributor

@andreaferretti and i think you now about HTTP expiration fields, because asynchttpserver and i think jester don't support it.

@andreaferretti
Contributor

@cheatfate The problem of using sendfile or epoll is that Rosencrantz would then only work on Linux, unlike asynchttpserver. This is (one of the reasons) why I am building on top of asynchttpserver.

About headers: the guiding philosophy of Rosencrantz is to provide HTTP combinators that allow to compose whatever response you want. I may provide helpers to fill expiration headers, but I will not set those fields - or any other - for the user, nor do content negotiation, etc. See this for a longer explanation

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment