code review 3991045: Use defer to unlock mutex in crypto/rand.

Use defer to unlock mutex in crypto/rand.

[anschelsc, 2011-01-21 14:23:38]

Hello golang-dev@googlegroups.com,

I'd like you to review this change.

[agl1, 2011-01-21 15:14:25]

LGTM

[agl1, 2011-01-21 15:15:05]

*** Submitted as http://code.google.com/p/go/source/detail?r=9a3609f2fa0c ***

Use defer to unlock mutex in crypto/rand.

R=agl1
CC=golang-dev
http://codereview.appspot.com/3991045

Committer: Adam Langley <agl@golang.org>

[peterGo, 2011-01-21 15:21:15]

Does Not LGTM

1. Locks should be released as soon as possible, not deferred.

2. By design, Windows uses the same pattern as Unix-like systems, so why wasn't
Windows changed too. 

3. Does Russ agree with the change to his fix?

On 2011/01/21 15:15:05, agl1 wrote:
> *** Submitted as http://code.google.com/p/go/source/detail?r=9a3609f2fa0c ***
> 
> Use defer to unlock mutex in crypto/rand.
> 
> R=agl1
> CC=golang-dev
> http://codereview.appspot.com/3991045
> 
> Committer: Adam Langley <mailto:agl@golang.org>

[cthom, 2011-01-21 15:25:20]

On 21 January 2011 10:21,  <go.peter.90@gmail.com> wrote:
> Does Not LGTM
>
> 1. Locks should be released as soon as possible, not deferred.

Both of those unlocks we're immediately followed by returns. Changing
it to a defer adds what? 2 extra jump instructions before the unlock?

> 2. By design, Windows uses the same pattern as Unix-like systems, so why
> wasn't Windows changed too.
>
> 3. Does Russ agree with the change to his fix?
>
> On 2011/01/21 15:15:05, agl1 wrote:
>>
>> *** Submitted as
>
> http://code.google.com/p/go/source/detail?r=9a3609f2fa0c ***
>
>> Use defer to unlock mutex in crypto/rand.
>
>> R=agl1
>> CC=golang-dev
>> http://codereview.appspot.com/3991045
>
>> Committer: Adam Langley <mailto:agl@golang.org>
>
>
>
> http://codereview.appspot.com/3991045/
>

[rog, 2011-01-21 15:28:11]

On 21 January 2011 15:25, Corey Thomasson <cthom.lists@gmail.com> wrote:
> On 21 January 2011 10:21,  <go.peter.90@gmail.com> wrote:
>> Does Not LGTM
>>
>> 1. Locks should be released as soon as possible, not deferred.
>
> Both of those unlocks we're immediately followed by returns. Changing
> it to a defer adds what? 2 extra jump instructions before the unlock?

it adds an allocation.
that can be significant, not that i'm saying that it is here.

[chressie1, 2011-01-21 15:54:12]

On Fri, Jan 21, 2011 at 4:25 PM, Corey Thomasson <cthom.lists@gmail.com> wrote:
> On 21 January 2011 10:21,  <go.peter.90@gmail.com> wrote:
>> Does Not LGTM
>>
>> 1. Locks should be released as soon as possible, not deferred.
>
> Both of those unlocks we're immediately followed by returns. Changing
> it to a defer adds what? 2 extra jump instructions before the unlock?

actually, the last return statement executes r.f.Read(), which happens
*before* the mutex is unlocked by the defer statement. this can
sometimes be an issue..

cheers,
chressie

[rsc, 2011-01-21 16:11:47]

LGTM

The lock was only strictly necessary
to protect against multiple Opens, but
I can't see that locking around the Read
does any harm, and it simplifies the code.
If we don't lock around the Read, the
kernel presumably will.  Since it's already
submitted, let's leave it at that.

Russ