fix: associate each `Session` with a `Channel` #1346

mr-salty · 2020-03-09T19:37:21Z

Factor the SessionPool::ChannelInfo struct out to a stand-alone
object, so we can store a shared_ptr to it in each Session.

This allows us to properly account for Sessions that are marked bad or
disassociated from the pool. Previously, the per-Channel session count
was not being decremented in those cases.

Since the Channel also holds a shared_ptr<Stub>, we can eliminate
the Session::stub_ member.

Fixes #1344

This change is

Factor the `SessionPool::ChannelInfo` struct out to a stand-alone object, so we can store a `shared_ptr` to it in each `Session`. This allows us to properly account for `Sessions` that are marked bad or disassociated from the pool. Previously, the per-Channel session count was not being decremented in those cases. Since the `Channel` also holds a `shared_ptr<Stub>`, we can eliminate the `Session::stub_` member. Fixes #1344

mr-salty · 2020-03-09T19:39:51Z

google/cloud/spanner/internal/channel.h, line 34 at r1 (raw file):

  explicit Channel(std::shared_ptr<SpannerStub> stub_param)
      : stub(std::move(stub_param)) {}

FWIW, I was debating disabling copy/move here because it's easy to make an inadvertent copy, and that causes some subtle bugs. Internally we tend to make things no copy/no move by default but don't really seem to do it in this project.

codecov · 2020-03-09T19:53:20Z

Codecov Report

Merging #1346 into master will decrease coverage by <.01%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #1346      +/-   ##
==========================================
- Coverage   94.65%   94.65%   -0.01%     
==========================================
  Files         171      173       +2     
  Lines       13637    13628       -9     
==========================================
- Hits        12908    12899       -9     
  Misses        729      729

Impacted Files	Coverage Δ
google/cloud/spanner/internal/session.h	`100% <ø> (ø)`	⬆️
google/cloud/spanner/internal/session_pool.h	`0% <ø> (-33.34%)`	⬇️
google/cloud/spanner/internal/session_pool.cc	`96.57% <100%> (+0.19%)`	⬆️
google/cloud/spanner/internal/channel.h	`100% <100%> (ø)`
google/cloud/spanner/internal/session.cc	`100% <100%> (ø)`	⬆️
google/cloud/spanner/internal/transaction_impl.h	`100% <0%> (ø)`	⬆️
...cloud/spanner/internal/partial_result_set_resume.h	`100% <0%> (ø)`	⬆️
google/cloud/spanner/session_pool_options.h	`100% <0%> (ø)`	⬆️
.../spanner/benchmarks/multiple_rows_cpu_benchmark.cc	`91.97% <0%> (ø)`	⬆️
google/cloud/spanner/transaction.h	`100% <0%> (ø)`	⬆️
... and 3 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 002096b...809d810. Read the comment docs.

google/cloud/spanner/internal/channel.h

google/cloud/spanner/internal/session.h

devbww

Reviewable status: 0 of 7 files reviewed, 4 unresolved discussions (waiting on @coryan and @mr-salty)

google/cloud/spanner/internal/channel.h, line 34 at r1 (raw file):

Previously, mr-salty (Todd Derr) wrote…

FWIW, I was debating disabling copy/move here because it's easy to make an inadvertent copy, and that causes some subtle bugs. Internally we tend to make things no copy/no move by default but don't really seem to do it in this project.

FWIW, having stub be const already does:

Channel& Channel::operator=(Channel const&) = delete;
Channel& Channel::operator=(Channel&&) = delete;

mr-salty

Reviewable status: 0 of 7 files reviewed, 4 unresolved discussions (waiting on @coryan and @devbww)

google/cloud/spanner/internal/channel.h, line 24 at r1 (raw file):

Previously, devbww (Bradley White) wrote…

#include "google/cloud/spanner/version.h"

Done.

google/cloud/spanner/internal/channel.h, line 31 at r1 (raw file):

Previously, devbww (Bradley White) wrote…

s/stub/stub_param/

Done.

google/cloud/spanner/internal/channel.h, line 34 at r1 (raw file):

Previously, devbww (Bradley White) wrote…

FWIW, having stub be const already does:

Channel& Channel::operator=(Channel const&) = delete;
Channel& Channel::operator=(Channel&&) = delete;

for assignment, but not construction... I definitely messed up trying to use it in one place.. so maybe I should just explicitly make it non-copy/move.

google/cloud/spanner/internal/session.h, line 19 at r1 (raw file):

Previously, devbww (Bradley White) wrote…

No longer needed?

Done.

coryan

The copyright notice needs to be fixed, the other thing is up to you.

Reviewed 7 of 7 files at r1, 2 of 2 files at r2.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @devbww and @mr-salty)

google/cloud/spanner/internal/channel.h, line 1 at r1 (raw file):

// Copyright 2019 Google LLC

s/2019/2020/

google/cloud/spanner/internal/channel.h, line 34 at r1 (raw file):

Previously, mr-salty (Todd Derr) wrote…

for assignment, but not construction... I definitely messed up trying to use it in one place.. so maybe I should just explicitly make it non-copy/move.

If disabling copy/move is better: go for it, this is in internal anyways. AFAIK we do not have a rule about making every type copyable. I have opinions about making user-facing code unnecessarily hard to use, but that is neither here nor there in this case.

mr-salty

Reviewable status: 6 of 7 files reviewed, 4 unresolved discussions (waiting on @coryan and @devbww)

google/cloud/spanner/internal/channel.h, line 1 at r1 (raw file):

Previously, coryan (Carlos O'Ryan) wrote…

s/2019/2020/

Done.

google/cloud/spanner/internal/channel.h, line 34 at r1 (raw file):

Previously, coryan (Carlos O'Ryan) wrote…

If disabling copy/move is better: go for it, this is in internal anyways. AFAIK we do not have a rule about making every type copyable. I have opinions about making user-facing code unnecessarily hard to use, but that is neither here nor there in this case.

ok, I made it non-copyable (and non-moveable)

devbww · 2020-03-09T21:12:32Z

google/cloud/spanner/internal/channel.h

+  Channel(const Channel&) = delete;
+  Channel& operator=(const Channel&) = delete;


East const.

devbww · 2020-03-09T21:14:07Z

google/cloud/spanner/internal/channel.h

+  explicit Channel(std::shared_ptr<SpannerStub> stub_param)
+      : stub(std::move(stub_param)) {}
+
+  // This class is not copyable or movable.


Channel(Channel&&) = delete; Channel& operator=(Channel&&) = delete;

as well?

coryan

once you address the questions from @devbww

Reviewed 1 of 1 files at r3.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @devbww and @mr-salty)

mr-salty

Reviewable status: 6 of 7 files reviewed, 5 unresolved discussions (waiting on @coryan and @devbww)

google/cloud/spanner/internal/channel.h, line 36 at r3 (raw file):

Previously, devbww (Bradley White) wrote…

  Channel(Channel&&) = delete;
  Channel& operator=(Channel&&) = delete;

as well?

not necessary: https://google.github.io/styleguide/cppguide.html#Copyable_Movable_Types

google/cloud/spanner/internal/channel.h, line 38 at r3 (raw file):

Previously, devbww (Bradley White) wrote…

East const.

%&&$&($*&

devbww

Reviewable status: 6 of 7 files reviewed, 5 unresolved discussions (waiting on @coryan, @devbww, and @mr-salty)

google/cloud/spanner/internal/channel.h, line 36 at r3 (raw file):

Previously, mr-salty (Todd Derr) wrote…

not necessary: https://google.github.io/styleguide/cppguide.html#Copyable_Movable_Types

Not necessary, but I believe we still choose to do it.

mr-salty · 2020-03-09T21:48:23Z

google/cloud/spanner/internal/channel.h, line 36 at r3 (raw file):

Previously, devbww (Bradley White) wrote…

Not necessary, but I believe we still choose to do it.

I see no documentation to support your assertion.

https://github.com/googleapis/google-cloud-cpp-common/blob/master/doc/cpp-style-guide.md

…ud-cpp-spanner#1346) * fix: associate each `Session` with a `Channel` Factor the `SessionPool::ChannelInfo` struct out to a stand-alone object, so we can store a `shared_ptr` to it in each `Session`. This allows us to properly account for `Sessions` that are marked bad or disassociated from the pool. Previously, the per-Channel session count was not being decremented in those cases. Since the `Channel` also holds a `shared_ptr<Stub>`, we can eliminate the `Session::stub_` member. Fixes googleapis/google-cloud-cpp-spanner#1344 * address review comments * address review comments * review comments

mr-salty requested review from coryan and devbww March 9, 2020 19:37

googlebot added the cla: yes This human has signed the Contributor License Agreement. label Mar 9, 2020

devbww reviewed Mar 9, 2020

View reviewed changes

google/cloud/spanner/internal/channel.h Show resolved Hide resolved

google/cloud/spanner/internal/channel.h Outdated Show resolved Hide resolved

google/cloud/spanner/internal/session.h Outdated Show resolved Hide resolved

devbww reviewed Mar 9, 2020

View reviewed changes

address review comments

d534999

mr-salty commented Mar 9, 2020

View reviewed changes

coryan suggested changes Mar 9, 2020

View reviewed changes

address review comments

a6a2cf1

mr-salty commented Mar 9, 2020

View reviewed changes

devbww reviewed Mar 9, 2020

View reviewed changes

coryan approved these changes Mar 9, 2020

View reviewed changes

review comments

809d810

mr-salty commented Mar 9, 2020

View reviewed changes

devbww reviewed Mar 9, 2020

View reviewed changes

mr-salty merged commit 4f2e66e into googleapis:master Mar 9, 2020

mr-salty deleted the sessioncount branch March 9, 2020 22:35

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: associate each `Session` with a `Channel` #1346

fix: associate each `Session` with a `Channel` #1346

mr-salty commented Mar 9, 2020 •

edited by coryan

Loading

mr-salty commented Mar 9, 2020

codecov bot commented Mar 9, 2020 •

edited

Loading

devbww left a comment

mr-salty left a comment

coryan left a comment

mr-salty left a comment

devbww Mar 9, 2020

devbww Mar 9, 2020

coryan left a comment

mr-salty left a comment

devbww left a comment

mr-salty commented Mar 9, 2020

		Channel(const Channel&) = delete;
		Channel& operator=(const Channel&) = delete;

fix: associate each Session with a Channel #1346

fix: associate each Session with a Channel #1346

Conversation

mr-salty commented Mar 9, 2020 • edited by coryan Loading

mr-salty commented Mar 9, 2020

codecov bot commented Mar 9, 2020 • edited Loading

Codecov Report

devbww left a comment

Choose a reason for hiding this comment

mr-salty left a comment

Choose a reason for hiding this comment

coryan left a comment

Choose a reason for hiding this comment

mr-salty left a comment

Choose a reason for hiding this comment

devbww Mar 9, 2020

Choose a reason for hiding this comment

devbww Mar 9, 2020

Choose a reason for hiding this comment

coryan left a comment

Choose a reason for hiding this comment

mr-salty left a comment

Choose a reason for hiding this comment

devbww left a comment

Choose a reason for hiding this comment

mr-salty commented Mar 9, 2020

fix: associate each `Session` with a `Channel` #1346

fix: associate each `Session` with a `Channel` #1346

mr-salty commented Mar 9, 2020 •

edited by coryan

Loading

codecov bot commented Mar 9, 2020 •

edited

Loading