Bug: Fork safe

[celogeek]

Hi,

Couchbase::Bucket is not fork safe as I can see.

my $pm = Parallel::ForkManager->new(8);
my $cb = Couchbase::Bucket->new("couchbase://127.0.0.1");
for(1..8) {
  $pm->start and next;
  $cb->fetch("test");
  $pm->finish;
}
$pm->wait_all_children;

### result :

[warn] kevent: Bad file descriptor
[warn] kevent: Bad file descriptor
[warn] kevent: Bad file descriptor
[warn] kevent: Bad file descriptor
[warn] kevent: Bad file descriptor
[warn] kevent: Bad file descriptor
[warn] kevent: Bad file descriptor
[warn] kevent: Bad file descriptor
``
`

The drivers should keep the main pid, and if it change, reconnect.

Tell me if I can help on something.

[mnunberg]

You cannot use the same library handle from different processes because you cannot use a single socket concurrently from multiple processes. This is common to all socket libraries.

[celogeek]

Yeah, I know. A lot of drivers support the fork process.

The general way to do is to keep the $PID inside the main class. When we want to use the socket, we check that the pid has changed. If it is the case, we close the connection, reopen it, and save the new pid.

This way fork are transparent.

I have make role to handle that around my connector, but I would suggest to add support in the client.

I can purpose a patch if you prefer.

[mnunberg]

I'm interested in seeing your solution, but this doesn't seem like something good. Can you show me an example of a driver that allows the behavior you're describing?

As an application developer you are supposed to make sure you don't share the same socket between multiple forks; and your code should be creating a new client handle inside the child process rather than attempting to "share" the client in the layer above.

As far as I know, none of the DBI drivers support this either. There is support for not inadvertently closing the connection in the parent so that an idle parent connection is usable in the child, but that's not what your code is doing. See https://metacpan.org/pod/DBI#InactiveDestroy

[ivulfson]

I agree with @mnunberg . DBI drivers don't support forking. It's a good idea and good practice for the developer to support forking in their code (disconnect, fork, reconnect).

[celogeek]

Hi, here some example :

https://metacpan.org/source/DAMS/Redis-1.979/lib/Redis.pm#L687 https://metacpan.org/source/WEBORAMA/Riak-Light-0.11/lib/Riak/Light.pm#L530

It ease a lot the forking process. You init your connector (at least the params) in the parent. You fork and the children reconnect them self.

Here my piece of code :

I have a class Connector.pm with

...

my $CURRENT_PID=$$;

has 'connector' => ( is => 'lazy', clearer => 1 );

sub _build_connector {
    my $self   = shift;
    my $config = $self->config;
    my $cb_hosts =
        'couchbase://'
      . join( ",", @{ $config->{hosts} } ) . '/'
      . $config->{bucket};
    return Couchbase::Bucket->new( $cb_hosts, $config->{options} );
}

around connector => sub {
    my $orig = shift;
    my $self = shift;
    if ( $CURRENT_PID != $$ ) {
        $CURRENT_PID = $$;
        $self->clear_connector;
        return $self->connector(@_);
    }
    else {
        return $self->$orig(@_);
    }
};

[mnunberg]

Ok, so some examples there are visible. Unfortunately with the way the client is structured, making it "fork-safe" is nearly impossible because most of the work is done in the C core.

I'll take your connector class into consideration when writing documentation, but I still don't think this is the right way to do things.. not to mention there's no guarantee that something in the child would not corrupt the stream of the parent.

[celogeek]

When I recreate a new Couchbase::Bucket, it should create a new connexion right ? so it should not be able to corrupt parent stream. Well I really hope so, or my code is buggy ;)