Drizzle

Merge lp://staging/~diego-fmpwizard/drizzle/bug-436685 into lp://staging/~drizzle-trunk/drizzle/development

  1. bug-436685
  2. Merge into development
Proposed by fmpwizard
Status: Superseded
Proposed branch: lp://staging/~diego-fmpwizard/drizzle/bug-436685
Merge into: lp://staging/~drizzle-trunk/drizzle/development
Diff against target: 43 lines
2 files modified
drizzled/drizzled.cc (+2/-1)
plugin/oldlibdrizzle/net_serv.cc (+11/-0)
To merge this branch: bzr merge lp://staging/~diego-fmpwizard/drizzle/bug-436685
Reviewer Review Type Date Requested Status
Eric Day (community) Approve
fmpwizard Pending
Review via email: mp+13196@code.staging.launchpad.net

This proposal supersedes a proposal from 2009-10-10.

This proposal has been superseded by a proposal from 2009-10-13.

To post a comment you must log in.
Revision history for this message
fmpwizard (diego-fmpwizard) wrote : Posted in a previous version of this proposal

Hi,

Fixed 3 of the stack traces found on bug#436685
"Drizzle crash when shutting down while having queries waiting on row locks"

After these changes, I was not able to reproduce any of the remaining crashes.

Thanks

 -Diego

Revision history for this message
Eric Day (eday) wrote : Posted in a previous version of this proposal

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. :)

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

* 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).

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

Thanks!
-Eric

review: Needs Fixing
Revision history for this message
fmpwizard (diego-fmpwizard) wrote : Posted in a previous version of this proposal

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

review: Needs Resubmitting
Revision history for this message
Eric Day (eday) wrote :

Thanks, looks good now!

Revision history for this message
Eric Day (eday) wrote :

Thanks, looks good now!

review: Approve
Revision history for this message
Jay Pipes (jaypipes) wrote :

Just a tiny style thing, Diego:

23 + /**
24 + * We could end up here with net->vio == NULL
25 + * See LP bug#436685
26 + * If that is the case, we exit the while loop
27 + */
28 + if (net->vio == NULL)
29 + break;
30 +

We use doxygenized comments only in function headers or when marking things like @todo...

:)

Revision history for this message
fmpwizard (diego-fmpwizard) wrote :

Thanks Eric,

Hi Jay,
See comments below:

On Mon, Oct 12, 2009 at 4:06 PM, Jay Pipes <email address hidden> wrote:
> Just a tiny style thing, Diego:

>
> We use doxygenized comments only in function headers or when marking things like @todo...

I have two questions then :)

1- What style should I use?
2- I wasn't even sure if I should include that comment (and the others
on the same patch), but I felt that they could help someone who would
come to this code in the near/far future. If there a better way to
specify things like this?

Thanks

  -Diego

>
> :)
> --
> https://code.launchpad.net/~diego-fmpwizard/drizzle/bug-436685/+merge/13196
> You are the owner of lp:~diego-fmpwizard/drizzle/bug-436685.
>

--
Diego Medina
Web Developer
http://www.fmpwizard.com

Revision history for this message
Jay Pipes (jaypipes) wrote :

fmpwizard wrote:
> Thanks Eric,
>
> Hi Jay,
> See comments below:
>
>
> On Mon, Oct 12, 2009 at 4:06 PM, Jay Pipes <email address hidden> wrote:
>> Just a tiny style thing, Diego:
>
>> We use doxygenized comments only in function headers or when marking things like @todo...
>
> I have two questions then :)
>
> 1- What style should I use?

/*
  *
  */

instead of:

/**
  *
  */

:)

> 2- I wasn't even sure if I should include that comment (and the others
> on the same patch), but I felt that they could help someone who would
> come to this code in the near/far future. If there a better way to
> specify things like this?

No, the comment was just fine, it just didn't need the /** prefix :)

Cheers!

Jay

> Thanks
>
> -Diego
>
>> :)
>> --
>> https://code.launchpad.net/~diego-fmpwizard/drizzle/bug-436685/+merge/13196
>> You are the owner of lp:~diego-fmpwizard/drizzle/bug-436685.
>>
>
>
>

lp://staging/~diego-fmpwizard/drizzle/bug-436685 updated
1169. By Diego Medina <email address hidden>

* Fixed comment format

Unmerged revisions

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'drizzled/drizzled.cc'
2--- drizzled/drizzled.cc 2009-10-07 08:14:19 +0000
3+++ drizzled/drizzled.cc 2009-10-13 02:24:12 +0000
4@@ -488,8 +488,9 @@
5 break;
6 }
7 tmp= session_list.front();
8+ /* Close before unlock, avoiding crash. See LP bug#436685 */
9+ tmp->client->close();
10 (void) pthread_mutex_unlock(&LOCK_thread_count);
11- tmp->client->close();
12 }
13 }
14
15
16=== modified file 'plugin/oldlibdrizzle/net_serv.cc'
17--- plugin/oldlibdrizzle/net_serv.cc 2009-09-23 19:09:31 +0000
18+++ plugin/oldlibdrizzle/net_serv.cc 2009-10-13 02:24:12 +0000
19@@ -530,6 +530,14 @@
20 assert(pos);
21 if ((long) (length= drizzleclient_vio_write(net->vio, pos, (size_t) (end-pos))) <= 0)
22 {
23+ /*
24+ * We could end up here with net->vio == NULL
25+ * See LP bug#436685
26+ * If that is the case, we exit the while loop
27+ */
28+ if (net->vio == NULL)
29+ break;
30+
31 const bool interrupted= drizzleclient_vio_should_retry(net->vio);
32 /*
33 If we read 0, or we were interrupted this means that
34@@ -610,6 +618,9 @@
35 /* First read is done with non blocking mode */
36 if ((long) (length= drizzleclient_vio_read(net->vio, pos, remain)) <= 0L)
37 {
38+ if (net->vio == NULL)
39+ goto end;
40+
41 const bool interrupted = drizzleclient_vio_should_retry(net->vio);
42
43 if (interrupted)