Patch: fix libeio cancellation

Marc Lehmann schmorp at schmorp.de
Thu Aug 28 05:41:30 CEST 2014


Sorry for the late reply.

On Tue, Aug 19, 2014 at 11:03:42AM +0200, Hongli Lai <hongli at phusion.nl> wrote:
> I don't know about IO::AIO. I am only talking about the libeio C library.
> 
> Here's how libeio loses the fd:

libeio can't lose the fd, as libeio doesn't own it. the caller creates the
fd and needs to take care of it. if you want it closede, you cna close it
in the destroy callback.

> 7. ETP_FINISH sees that the cancel flag is 1, and returns. The
> callback is never called, and libeio never attempts to close the file
> descriptor returned by open(). The fd is now lost.

libeio never closes fds for you unless you explicitly ask it to (via
EIO_CLOSE).

> > Same thing, I don't see a lot of this in IO::AIO, in both cases, the
> > resources will be freed when the request is destroyed.
> 
> I don't think we're talking about the same thing. It isn't a problem
> in Perl because everything's garbage collected, but I'm not talking
> about the Perl IO::AIO module, I'm talking about the libeio C library.

Perl *uses* the libeio C library, so whatever Perl does, you can do
yourself. Besides, Perl doesn't garbage collect "everything", and most
certainly, it doesn't garbage collect eio requests or their memory, as it has
no knowledge about it.

What IO::AIO does is simply take a reference to the filehandle object of
Perl, to keep it from garbage collecting it. When the request is freed, it
also "frees" that reference, after which Perl might garbage collect it or
not.

Garbage colelction doesn't work that differently than malloc/free -
instead of malloc, you take a reference/keep a pointer, instead of free,
you destroy the reference/the pointer.

So if you have per-request memory, you malloc it before eio_submit and free
it in the destroy callback. If the memory is tied to another object, you need
to do reference counting or use some other garbage collection technique to
keep it from being freed prematurely.

Always calling the request callback will not do anything, as the request
callback and the destroy callback are (at least currently) called at the
same time in the same thread.

> Libeio does not free the user data. I have to do it myself.

Right, libeio only manages it's own resources.

> Then, when some event occurs (e.g. user clicks on Cancel or something):
> 
>     foo_destroy(foo);
> 
> If I don't call the free()s inside nop_done, then the malloc()'ed
> UserData and the strdupped "hello" string are lost, and we have a
> memory leak.

The solution is to call free on the memory - I am not sure what you are
after.

> But consider the current situation, in which libeio might or might not
> call the callback after cancellation. My code would have to become
> this:

Your example doesn't show anything about how you use these callbacks, I
assume that nop_done is the request callback, and nop_destroy is called
form the destroy callback. Then:

>         // ADDED: because foo_destroy() might already have cleaned
>         // things up.
>         if (data == NULL) {
>             return 0;
>         }

No, it might not. libeio *never* destroys your request and *then* calls
the request callback. That would be an obvious bug as the memory is
already freed.

>         // 'foo' might be a dangling pointer if we're canceled
>         if (!EIO_CANCELLED(req)) {
>             foo->request = NULL;
>         }

This would only be needed with your change. In libeio the above condition
is always falsed, and trhe whole block is dead code.

>     void foo_destroy(Foo *foo) {
>         if (foo->request != NULL) {
>             eio_cancel(foo->request);

It makes no sense to cancel the request when it is being destroyed - when the
destroy callback is called, the request is done, for better or worse.

You can just unconditionally call free on your data - after submitting your
request via eio_submit, the destroy callback *will* be called, whether the
request callback is called or not.

If you put all your free's in the destroy callback, then there is no need
for if's anywhere.

> Notice the ADDED lines. Now, I suddenly have to add free() calls to
> foo_destroy().

Well, you could have them in destroy always - it's called almost instantly
after the request callback.

> I can no longer assume that the nop_done() is the only
> place where I need to free things.

You could never assume that, really.

> Also, in nop_done(), I need to check whether foo_destroy() hasn't
> already cleaned things up.

I don't need to do that in IO::AIO, so if you need to, then this is not a
requirement of libeio but something else in your code, which I don't know
about.

> This is a simple example so the added code isn't too much, but in more
> complex real-world code, having to free things from two places quickly
> becomes a mess.

Well, your example doesn't show why you need to free things from two
places:  I can only point at IO::AIO, which doesn't need that and uses the
same API as is available to you.

-- 
                The choice of a       Deliantra, the free code+content MORPG
      -----==-     _GNU_              http://www.deliantra.net
      ----==-- _       generation
      ---==---(_)__  __ ____  __      Marc Lehmann
      --==---/ / _ \/ // /\ \/ /      schmorp at schmorp.de
      -=====/_/_//_/\_,_/ /_/\_\



More information about the libev mailing list