pgwire: fix memory account leak

[andreimatei]

Want unique_ptr.

Release note: None

[cockroach-teamcity]

This change is 

[knz]

Good catch.

Want unique_ptr.

I feel you!

---

Reviewed 1 of 1 files at r1.
Review status: all files reviewed at latest revision, 1 unresolved discussion, some commit checks pending.

---

pkg/sql/pgwire/v3.go, line 340 at r1 (raw file):

		c.writeBuf.writeTerminatedString(value)
		if err := c.writeBuf.finishMsg(c.wr); err != nil {
			reserved.Close(ctx)

For good measure, add an explanatory comment that these Closes are not needed any more once the defer underneath has been set up, because the session then takes ownership of the thing.

---

Comments from Reviewable

[petermattis]

Review status: all files reviewed at latest revision, 2 unresolved discussions, some commit checks pending.

---

pkg/sql/pgwire/v3.go, line 340 at r1 (raw file):

		c.writeBuf.writeTerminatedString(value)
		if err := c.writeBuf.finishMsg(c.wr); err != nil {
			reserved.Close(ctx)

Seems more natural to do this in the caller, we just have to make BoundAccount.Close idempotent, but that isn't difficult. Actually, BoundAccount.Clear already does the right thing. I don't see a particular reason for the difference between BoundAccount.Clear and BoundAccount.Close.

---

Comments from Reviewable

[petermattis]

PS No test?

---

Review status: all files reviewed at latest revision, 2 unresolved discussions, some commit checks pending.

---

Comments from Reviewable

[andreimatei]

A test would probably be complicated to write... We need a connection that fails at just the right time...
Plus, this particular code (v3Conn) is going away. Not that I'll put a test for the equivalent thing in the new code :P

I think the only hope for catching such things is a high-level test that bombards a server with connections.

---

Review status: all files reviewed at latest revision, 2 unresolved discussions, all commit checks successful.

---

pkg/sql/pgwire/v3.go, line 340 at r1 (raw file):

Previously, petermattis (Peter Mattis) wrote…

Seems more natural to do this in the caller, we just have to make BoundAccount.Close idempotent, but that isn't difficult. Actually, BoundAccount.Clear already does the right thing. I don't see a particular reason for the difference between BoundAccount.Clear and BoundAccount.Close.

Starting to do this sort of thing in the callers generally seems like a bad idea to me. The sane contract is callee takes responsibility. The moment control flow is returned to the caller shouldn't matter as far as the resource is concerned. If the callee wants to pass ownership along to an async fella, it's her business.

---

pkg/sql/pgwire/v3.go, line 340 at r1 (raw file):

Previously, knz (kena) wrote…

For good measure, add an explanatory comment that these Closes are not needed any more once the defer underneath has been set up, because the session then takes ownership of the thing.

ummm sounds like it'd be a pretty rando comment. On which line would I even put it?
Technically it's not defer that takes away the need for closing the account, it's the setupSession() call - which, in turn, mandates a closeSession() call.
The contract with these accounts everywhere is that their ownership gets passed along; we're not gonna comment that everywhere.

---

Comments from Reviewable

[knz]

Review status: all files reviewed at latest revision, 2 unresolved discussions, all commit checks successful.

---

pkg/sql/pgwire/v3.go, line 340 at r1 (raw file):

Previously, andreimatei (Andrei Matei) wrote…

ummm sounds like it'd be a pretty rando comment. On which line would I even put it?
Technically it's not defer that takes away the need for closing the account, it's the setupSession() call - which, in turn, mandates a closeSession() call.
The contract with these accounts everywhere is that their ownership gets passed along; we're not gonna comment that everywhere.

All right, carry on.

---

Comments from Reviewable

[petermattis]

A test would probably be complicated to write... We need a connection that fails at just the right time...
Plus, this particular code (v3Conn) is going away. Not that I'll put a test for the equivalent thing in the new code :P

Let me push back on the difficulty: a test could manually create a v3Conn and precisely control the net connection. Triggering at least one of these error paths doesn't seem difficult.

---

Review status: all files reviewed at latest revision, 3 unresolved discussions, all commit checks successful.

---

pkg/sql/pgwire/v3.go, line 340 at r1 (raw file):

Previously, andreimatei (Andrei Matei) wrote…

Starting to do this sort of thing in the callers generally seems like a bad idea to me. The sane contract is callee takes responsibility. The moment control flow is returned to the caller shouldn't matter as far as the resource is concerned. If the callee wants to pass ownership along to an async fella, it's her business.

Ok, at the top of this function, defer func() { reserved.Close(ctx) }(). After the session has been created successfully, reserved = mon.BoundAccount{}.

---

pkg/sql/pgwire/v3.go, line 351 at r1 (raw file):

	ctx = log.WithLogTagStr(ctx, "user", c.sessionArgs.User)
	if err := c.setupSession(ctx, reserved); err != nil {
		return err

I think you missed a reserved.Close(ctx) here.

---

Comments from Reviewable

[petermattis]

PS I can understand not wanting to test in this soon to be obsolete code, I was trying to motivate testing in the new code.

---

Review status: all files reviewed at latest revision, 2 unresolved discussions, all commit checks successful.

---

Comments from Reviewable

[andreimatei]

Review status: all files reviewed at latest revision, 3 unresolved discussions, all commit checks successful.

---

pkg/sql/pgwire/v3.go, line 340 at r1 (raw file):

Previously, petermattis (Peter Mattis) wrote…

Ok, at the top of this function, defer func() { reserved.Close(ctx) }(). After the session has been created successfully, reserved = mon.BoundAccount{}.

nit: no need for the anonymous function, the capture would work the same with defer reserved.Close(ctx), right?

That is indeed the alternative, but I for one don't prefer it - then instead of worrying about whether I'm missing a Close(), I have to worry about missing to reset a rando variable. It's true that generally the reset only needs to happen once, but still that's too easy to miss.
Having said that, I believe I've used your pattern on occasions too, so I don't have too strong feelings. Educate me more if you care, pls.

My favorite option is still what I once sent to you and you've rejected with prejudice: a unique_ptr struct that wraps a Closer and has a Close() and a Release(). Release would make future Close a no-op, and you'd defer the Close() and use Release() when you pass the resource away.

---

pkg/sql/pgwire/v3.go, line 351 at r1 (raw file):

Previously, petermattis (Peter Mattis) wrote…

I think you missed a reserved.Close(ctx) here.

To this I say - no again. The sane contract is that the callee always takes ownership, regardless of whether it returns an error or not. This is implemented naturally by callees that can afford to defer Close() in the beginning.
Whether this setupSession() is correct with regards to that I don't really care to find out, because that code is going away too.

We could make acc.Close() idempotent and then we could both win here. I'm not sure how I fell about that.

---

Comments from Reviewable

[andreimatei]

Review status: all files reviewed at latest revision, 3 unresolved discussions, all commit checks successful.

---

pkg/sql/pgwire/v3.go, line 351 at r1 (raw file):

We could make acc.Close() idempotent and then we could both win here. I'm not sure how I fell about that.

meaning, I'd agree that contracts on errors returned in Go are more often ad-hoc.

---

Comments from Reviewable

[petermattis]

let's not get caught up in an argument over dead code. My larger concern is how easy it is to get the memory accounting wrong. Our patterns for adding memory accounting are not robust.

---

Review status: all files reviewed at latest revision, 3 unresolved discussions, all commit checks successful.

---

pkg/sql/pgwire/v3.go, line 340 at r1 (raw file):

Previously, andreimatei (Andrei Matei) wrote…

nit: no need for the anonymous function, the capture would work the same with defer reserved.Close(ctx), right?

That is indeed the alternative, but I for one don't prefer it - then instead of worrying about whether I'm missing a Close(), I have to worry about missing to reset a rando variable. It's true that generally the reset only needs to happen once, but still that's too easy to miss.
Having said that, I believe I've used your pattern on occasions too, so I don't have too strong feelings. Educate me more if you care, pls.

My favorite option is still what I once sent to you and you've rejected with prejudice: a unique_ptr struct that wraps a Closer and has a Close() and a Release(). Release would make future Close a no-op, and you'd defer the Close() and use Release() when you pass the resource away.

I think the function arguments (include the receiver) are captured when the defer is created.

You only have to reset the rando variable in one place vs ensuring you call reserved.Close in N places (N currently equals 2).

I don't recall previously rejecting a unique_ptr idea, though trying to add a unique_ptr to Go seems like an exercise in frustration. What you're describing is a bit different and is effectively what is accomplished with my suggestion here.

---

pkg/sql/pgwire/v3.go, line 351 at r1 (raw file):

Previously, andreimatei (Andrei Matei) wrote…

We could make acc.Close() idempotent and then we could both win here. I'm not sure how I fell about that.

meaning, I'd agree that contracts on errors returned in Go are more often ad-hoc.

Well, setupSession currently never returns an error so this discussion is moot.

---

Comments from Reviewable

[andreimatei]

Review status: all files reviewed at latest revision, 3 unresolved discussions, all commit checks successful.

---

pkg/sql/pgwire/v3.go, line 340 at r1 (raw file):

I think the function arguments (include the receiver) are captured when the defer is created.

FWIW seems to work without the anon func:

https://play.golang.org/p/in2EvKo7zPH

You only have to reset the rando variable in one place vs ensuring you call reserved.Close in N places (N currently equals 2).

Maybe, maybe not. Sometimes you can alternatively transfer ownership on different branches.

I don't recall previously rejecting a unique_ptr idea, though trying to add a unique_ptr to Go seems like an exercise in frustration. What you're describing is a bit different and is effectively what is accomplished with my suggestion here.

I'll reintroduce it with the following occasion in the 2018 proceedings and I'l quote you on this.

---

Comments from Reviewable