Code review comment for lp://staging/~diego-fmpwizard/drizzle/bug-436685

Hi Eric,

Thanks for reviewing this.

> Hi!
>
> Good catch on the bug. This was a bit of a hack to force the closing of the
> socket until shutdown could be cleaned up some more and we can remove
> oldlibdrizzle entirely and use the new libdrizzle. :)

Now I see why this plugin has such an odd name :)

>
> Two things:
>
> * I'm not sure if we need to move the tmp->client->close(); call inside the
> lock, I don't think that gets use anything
>

I reverted this and I found it necessary. And I have a very simple way to reproduce it:

1- Start drizzled
2- Start one drizzle client
$ ./bin/drizzle -p3306 (I changed the port number on drizzle.cnf)

3- Start another drizzle client
$ ./bin/drizzle -p3306

4- Shutdown drizzle like this:
$ ./bin/drizzle -p3306 --shutdown

5- See drizzled crash (if you were using gdb, you will see it crashing on tmp->client->close();

> * The vio.cc changes are not needed (and that last line does nothing). At the
> top of drizzleclient_vio_delete vio is checked to be sure it's not NULL, so
> your != NULL check would be an extremely rare race condition, if at all (since
> the op right above would dereference vio for vio->read_buffer).
>

I am 99% sure I saw vio turned NULL right before free((unsigned char*) vio); , but I could not reproduce it any more today, so I reverted vio.cc.
If I happen to hit this condition again, I'll just open a new bug.

> So, I would definitely revert the vio.cc change, and test to see if the
> drizzled.cc changes are really needed.
>

Thanks

  -Diego

> Thanks!
> -Eric