Mir

Merge lp:~alan-griffiths/mir/fix-1486496 into lp:mir

fix-1486496
Merge into development-branch

Proposed by Alan Griffiths on 2015-08-21

Status:

Superseded

Proposed branch:

lp:~alan-griffiths/mir/fix-1486496

Merge into:

lp:mir

Prerequisite:

lp:~alan-griffiths/mir/simplify-connect-code

Diff against target:

283 lines (+136/-7)

9 files modified

src/client/mir_connection.cpp (+1/-1)
src/client/rpc/mir_basic_rpc_channel.cpp (+24/-2)
src/client/rpc/mir_basic_rpc_channel.h (+1/-0)
src/include/common/mir/frontend/client_constants.h (+3/-1)
src/include/common/mir/protobuf/protocol_version.h (+48/-0)
src/server/frontend/socket_connection.cpp (+8/-1)
tests/acceptance-tests/test_client_library.cpp (+48/-0)
tests/integration-tests/test_error_reporting.cpp (+1/-1)
tests/unit-tests/frontend/test_socket_connection.cpp (+2/-1)

To merge this branch:

bzr merge lp:~alan-griffiths/mir/fix-1486496

Medium

Fix Released

Link a bug report

Reviewer	Review Type	Date Requested	Status
PS Jenkins bot	continuous-integration		Needs Fixing on 2015-08-24
Alan Griffiths			Abstain on 2015-08-24
Chris Halse Rogers		2015-08-21	Needs Fixing on 2015-08-24
Review via email: mp+268747@code.launchpad.net

Commit Message

client, frontend: Introduce meaningful versioning into the protobuf protocol and check it.

Description of the Change

client, frontend: Introduce meaningful versioning into the protobuf protocol and check it.

Because the server simply disconnects on a connection failure the client doesn't receive any reason for the failure. However, reporting a connection failure is better than hanging.

PS Jenkins bot (ps-jenkins) wrote on 2015-08-21:

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/mir-ci/4645/rebuild

review: Needs Fixing (continuous-integration)

lp:~alan-griffiths/mir/fix-1486496 updated on 2015-08-21

2873. By Alan Griffiths on 2015-08-21: Release connection object after connect errors

PS Jenkins bot (ps-jenkins) wrote on 2015-08-21:

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/mir-ci/4647/rebuild

review: Needs Fixing (continuous-integration)

lp:~alan-griffiths/mir/fix-1486496 updated on 2015-08-21

2874. By Alan Griffiths on 2015-08-21: merge lp:mir

PS Jenkins bot (ps-jenkins) wrote on 2015-08-21:

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/mir-ci/4649/rebuild

review: Needs Fixing (continuous-integration)

lp:~alan-griffiths/mir/fix-1486496 updated on 2015-08-22

2875. By Alan Griffiths on 2015-08-22: Remove debug code

PS Jenkins bot (ps-jenkins) wrote on 2015-08-22:

FAILED: Continuous integration, rev:2875
http://jenkins.qa.ubuntu.com/job/mir-ci/4651/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-vivid-i386-build/3635
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/mir-clang-ts-vivid-amd64-build/56
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-vivid-amd64-build/2546
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-vivid-touch/3585/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-wily-amd64-ci/800/console
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/3585
        deb: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/3585/artifact/work/output/*zip*/output.zip
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-runner-mako/6354/console
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/22763

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/mir-ci/4651/rebuild

review: Needs Fixing (continuous-integration)

Chris Halse Rogers (raof) wrote on 2015-08-24:

126 +// Client libraries older than last ABI break may make calls we can't understand

This seems to be an extremely conservative choice, which will likely prevent many clients which would otherwise work fine from connecting. We remove protocol extremely rarely; the last protocol we removed had been deprecated for 2 years.

I don't think it's unreasonable to manually populate the oldest_compatible_protocol_version().

Otherwise sensible.

review: Needs Fixing

Alan Griffiths (alan-griffiths) wrote on 2015-08-24:

> 126 +// Client libraries older than last ABI break may make calls we can't
> understand
>
> This seems to be an extremely conservative choice, which will likely prevent
> many clients which would otherwise work fine from connecting. We remove
> protocol extremely rarely; the last protocol we removed had been deprecated
> for 2 years.

If you investigate the linked bug you'll find that this choice is actually correct as things stand on trunk.

I don't think it is *yet* worth spending the effort to ensure current libmirclient can interwork with old libmirserver versions. (There is no likelihood of Mir servers based on different libmirservers being installed side-by-side on a user system.)

> I don't think it's unreasonable to manually populate the
> oldest_compatible_protocol_version().

I think being conservative is a good default.

Next time we bump the major API version number we can consider if we can be less conservative. I hope after all the "opaquification" and deprecation that landed in 0.14 that this will be a very long time.

> Otherwise sensible.

review: Approve

Alan Griffiths (alan-griffiths) wrote on 2015-08-24:

Oops!

review: Abstain

Alan Griffiths (alan-griffiths) wrote on 2015-08-24:

After attempting to reproduce locally and chatting with kdub on IRC it looks like the CI failure (hang in glmark) is related to lp:1441553.

lp:~alan-griffiths/mir/fix-1486496 updated on 2015-08-24

2876. By Alan Griffiths on 2015-08-24: kdub's workaround for client/server allocation mismatch
2877. By Alan Griffiths on 2015-08-24: merge lp:mir

PS Jenkins bot (ps-jenkins) wrote on 2015-08-24:

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/mir-ci/4661/rebuild

review: Needs Fixing (continuous-integration)

lp:~alan-griffiths/mir/fix-1486496 updated on 2015-08-24

2878. By Alan Griffiths on 2015-08-24: Revert failed workaround

PS Jenkins bot (ps-jenkins) wrote on 2015-08-24:

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/mir-ci/4666/rebuild

review: Needs Fixing (continuous-integration)

Alan Griffiths (alan-griffiths) wrote on 2015-08-25:

The hang in mir_performance test is because this...

Get:3 http://ports.ubuntu.com/ubuntu-ports/ vivid/universe glmark2-es2-mir armhf 2014.03-0ubuntu3 [298 kB]

... is the libmirclient.so.8 based version from vivid. *Not* the libmirclient.so.9 based rebuild from the "Stable Phone Overlay PPA".

(That is version 2014.03+git20150611.fa71af2d-0ubuntu1.1~overlay1)

lp:~alan-griffiths/mir/fix-1486496 updated on 2015-08-28

2879. By Alan Griffiths on 2015-08-25: merge lp:mir
2880. By Alan Griffiths on 2015-08-25: merge lp:~alan-griffiths/mir/workaround-1441553
2881. By Alan Griffiths on 2015-08-27: Add an epoch to the protocol version number
2882. By Alan Griffiths on 2015-08-27: merge lp:mir
2883. By Alan Griffiths on 2015-08-28: Marginally more informative error message
2884. By Alan Griffiths on 2015-08-28: merge lp:mir

Unmerged revisions

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk

Subscribers

People subscribed via source and target branches

to all changes:

Alan Griffiths

Emanuele Antonio Faraone

Gerry Boland

Mir development team

 === modified file 'src/client/mir_connection.cpp'
 --- src/client/mir_connection.cpp	2015-08-21 11:36:32 +0000
 +++ src/client/mir_connection.cpp	2015-08-25 15:45:40 +0000
@@ -165,7 +165,7 @@
+ {
      std::lock_guard<decltype(mutex)> lock(mutex);
--    if (connect_result && connect_result->has_error())
++    if (error_message.empty() && connect_result)
+     {
          return connect_result->error().c_str();
+     }
 === modified file 'src/client/rpc/mir_basic_rpc_channel.cpp'
 --- src/client/rpc/mir_basic_rpc_channel.cpp	2015-07-29 01:39:52 +0000
 +++ src/client/rpc/mir_basic_rpc_channel.cpp	2015-08-25 15:45:40 +0000
@@ -23,12 +23,33 @@
  #include "mir_protobuf_wire.pb.h"
  #include "mir/frontend/client_constants.h"
  #include "mir/variable_length_array.h"
++#include "mir/protobuf/protocol_version.h"
++#include "mir/log.h"
  #include <sstream>
  namespace mclr = mir::client::rpc;
  namespace mclrd = mir::client::rpc::detail;
++namespace
++{
++int get_protocol_version()
++{
++    auto protocol_version = mir::protobuf::current_protocol_version();
++
++    if (auto const protocol_version_override = getenv("MIR_CLIENT_TEST_OVERRRIDE_PROTOCOL_VERSION"))
++    {
++        int const new_protcol_version = strtol(protocol_version_override, nullptr, 0);
++        mir::log(mir::logging::Severity::warning, MIR_LOG_COMPONENT,
++                 "Overriding protocol version 0x%x with 0x%x", protocol_version, new_protcol_version);
++
++        protocol_version = new_protcol_version;
++    }
++
++    return protocol_version;
++}
++}
++
  mclrd::PendingCallCache::PendingCallCache(
      std::shared_ptr<RpcReport> const& rpc_report)
      : rpc_report{rpc_report}
@@ -99,7 +120,8 @@
  mclr::MirBasicRpcChannel::MirBasicRpcChannel() :
--    next_message_id(0)
++    next_message_id(0),
++    protocol_version{get_protocol_version()}
+ {
+ }
@@ -120,7 +142,7 @@
      invoke.set_id(next_id());
      invoke.set_method_name(method_name);
      invoke.set_parameters(buffer.data(), buffer.size());
--    invoke.set_protocol_version(1);
++    invoke.set_protocol_version(protocol_version);
      invoke.set_side_channel_fds(num_side_channel_fds);
      return invoke;
 === modified file 'src/client/rpc/mir_basic_rpc_channel.h'
 --- src/client/rpc/mir_basic_rpc_channel.h	2015-07-29 01:39:52 +0000
 +++ src/client/rpc/mir_basic_rpc_channel.h	2015-08-25 15:45:40 +0000
@@ -118,6 +118,7 @@
  private:
      std::atomic<int> next_message_id;
++    int const protocol_version;
  };
+ }
 === modified file 'src/include/common/mir/frontend/client_constants.h'
 --- src/include/common/mir/frontend/client_constants.h	2015-02-22 07:46:25 +0000
 +++ src/include/common/mir/frontend/client_constants.h	2015-08-25 15:45:40 +0000
@@ -27,7 +27,9 @@
  /// Number of buffers the client library will keep.
  /// mir::client::ClientBufferDepository and mir::frontend::ClientBufferTracker need to use the same value
--unsigned int const client_buffer_cache_size = 3;
++// TODO this ought to be 3 - but is 4 as a workaround for issues with overallocation (lp:1441553)
++// (this should become moot with "new Buffer Semantics")
++unsigned int const client_buffer_cache_size = 4;
  /// Buffers need to be big enough to support messages
  unsigned int const serialization_buffer_size = 2048;
 === added file 'src/include/common/mir/protobuf/protocol_version.h'
 --- src/include/common/mir/protobuf/protocol_version.h	1970-01-01 00:00:00 +0000
 +++ src/include/common/mir/protobuf/protocol_version.h	2015-08-25 15:45:40 +0000
@@ -0,0 +1,48 @@
++/*
++ * Copyright © 2015 Canonical Ltd.
++ *
++ * This program is free software: you can redistribute it and/or modify it
++ * under the terms of the GNU Lesser General Public License version 3,
++ * as published by the Free Software Foundation.
++ *
++ * This program is distributed in the hope that it will be useful,
++ * but WITHOUT ANY WARRANTY; without even the implied warranty of
++ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
++ * GNU Lesser General Public License for more details.
++ *
++ * You should have received a copy of the GNU Lesser General Public License
++ * along with this program.  If not, see <http://www.gnu.org/licenses/>.
++ *
++ * Authored by: Alan Griffiths <alan@octopull.co.uk>
++ */
++
++#ifndef MIR_PROTOBUF_PROTOCOL_VERSION_H
++#define MIR_PROTOBUF_PROTOCOL_VERSION_H
++
++#include "mir_toolkit/version.h"
++
++namespace mir
++{
++namespace protobuf
++{
++inline constexpr int protocol_version(int major, int minor)
++{
++    return (major << 10) + minor;
++}
++
++// For the present we use the client API protocol_version as a proxy for the protocol
++//  protocol_version as the protocol is typically updated to support API changes.
++inline constexpr int current_protocol_version()
++{
++    return protocol_version(MIR_CLIENT_MAJOR_VERSION, MIR_CLIENT_MINOR_VERSION);
++}
++
++// Client libraries older than last ABI break may make calls we can't understand
++inline constexpr int oldest_compatible_protocol_version()
++{
++    return protocol_version(MIR_CLIENT_MAJOR_VERSION, 0);
++}
++}
++}
++
++#endif //MIR_PROTOBUF_PROTOCOL_VERSION_H
 === modified file 'src/server/frontend/socket_connection.cpp'
 --- src/server/frontend/socket_connection.cpp	2015-02-22 07:46:25 +0000
 +++ src/server/frontend/socket_connection.cpp	2015-08-25 15:45:40 +0000
@@ -21,6 +21,7 @@
  #include "message_receiver.h"
  #include "mir/frontend/message_processor.h"
  #include "mir/frontend/session_credentials.h"
++#include "mir/protobuf/protocol_version.h"
  #include "mir_protobuf_wire.pb.h"
@@ -90,6 +91,10 @@
  void mfd::SocketConnection::on_new_message(const boost::system::error_code& error)
  try
+ {
++    // No support for newer client libraries
++    static auto const forward_compatibility_limit = mir::protobuf::current_protocol_version();
++    static auto const backward_compatibility_limit = mir::protobuf::oldest_compatible_protocol_version();
++
      if (error)
+     {
          BOOST_THROW_EXCEPTION(std::runtime_error(error.message()));
@@ -98,7 +103,9 @@
      mir::protobuf::wire::Invocation invocation;
      invocation.ParseFromArray(body.data(), body.size());
--    if (!invocation.has_protocol_version() || invocation.protocol_version() != 1)
++    if (!invocation.has_protocol_version() ||
++        invocation.protocol_version() > forward_compatibility_limit ||
++        invocation.protocol_version() < backward_compatibility_limit)
          BOOST_THROW_EXCEPTION(std::runtime_error("Unsupported protocol version"));
      std::vector<mir::Fd> fds;
 === modified file 'tests/acceptance-tests/test_client_library.cpp'
 --- tests/acceptance-tests/test_client_library.cpp	2015-07-27 07:24:50 +0000
 +++ tests/acceptance-tests/test_client_library.cpp	2015-08-25 15:45:40 +0000
@@ -136,6 +136,8 @@
      mtf::UsingStubClientPlatform using_stub_client_platform;
  };
++
++auto const* const protocol_version_override = "MIR_CLIENT_TEST_OVERRRIDE_PROTOCOL_VERSION";
+ }
  using namespace testing;
@@ -164,6 +166,52 @@
      mir_connection_release(connection);
+ }
++TEST_F(ClientLibrary, connects_when_protobuf_protocol_oldest_supported)
++{
++    std::ostringstream buffer;
++    buffer << ((MIR_CLIENT_MAJOR_VERSION) << 10);
++
++    add_to_environment(protocol_version_override, buffer.str().c_str());
++
++    connection = mir_connect_sync(new_connection().c_str(), __PRETTY_FUNCTION__);
++
++    EXPECT_THAT(connection, NotNull());
++    EXPECT_TRUE(mir_connection_is_valid(connection));
++    EXPECT_THAT(mir_connection_get_error_message(connection), StrEq(""));
++
++    mir_connection_release(connection);
++}
++
++TEST_F(ClientLibrary, reports_error_when_protobuf_protocol_obsolete)
++{
++    std::ostringstream buffer;
++    buffer << ((MIR_CLIENT_MAJOR_VERSION-1) << 10);
++    add_to_environment(protocol_version_override, buffer.str().c_str());
++
++    connection = mir_connect_sync(new_connection().c_str(), __PRETTY_FUNCTION__);
++
++    EXPECT_THAT(connection, NotNull());
++    EXPECT_FALSE(mir_connection_is_valid(connection));
++    EXPECT_THAT(mir_connection_get_error_message(connection), StrEq("Connect failed"));
++
++    mir_connection_release(connection);
++}
++
++TEST_F(ClientLibrary, reports_error_when_protobuf_protocol_too_new)
++{
++    std::ostringstream buffer;
++    buffer << ((MIR_CLIENT_MAJOR_VERSION) << 10) + MIR_CLIENT_MINOR_VERSION+1;
++    add_to_environment(protocol_version_override, buffer.str().c_str());
++
++    connection = mir_connect_sync(new_connection().c_str(), __PRETTY_FUNCTION__);
++
++    EXPECT_THAT(connection, NotNull());
++    EXPECT_FALSE(mir_connection_is_valid(connection));
++    EXPECT_THAT(mir_connection_get_error_message(connection), StrEq("Connect failed"));
++
++    mir_connection_release(connection);
++}
++
  TEST_F(ClientLibrary, creates_surface)
+ {
      mir_wait_for(mir_connect(new_connection().c_str(), __PRETTY_FUNCTION__, connection_callback, this));
 === modified file 'tests/integration-tests/test_error_reporting.cpp'
 --- tests/integration-tests/test_error_reporting.cpp	2015-07-29 01:18:29 +0000
 +++ tests/integration-tests/test_error_reporting.cpp	2015-08-25 15:45:40 +0000
@@ -100,7 +100,7 @@
      ASSERT_TRUE(connection != NULL);
      EXPECT_FALSE(mir_connection_is_valid(connection));
--    EXPECT_THAT(mir_connection_get_error_message(connection), testing::HasSubstr(test_exception_text));
++    EXPECT_THAT(mir_connection_get_error_message(connection), testing::HasSubstr("Connect failed"));
      mir_connection_release(connection);
+ }
 === modified file 'tests/unit-tests/frontend/test_socket_connection.cpp'
 --- tests/unit-tests/frontend/test_socket_connection.cpp	2015-06-25 03:00:08 +0000
 +++ tests/unit-tests/frontend/test_socket_connection.cpp	2015-08-25 15:45:40 +0000
@@ -22,6 +22,7 @@
  #include "mir/frontend/message_processor.h"
  #include "mir/frontend/session_credentials.h"
  #include "mir/fd.h"
++#include "mir/protobuf/protocol_version.h"
  #include "mir/test/fake_shared.h"
@@ -145,7 +146,7 @@
          invocation.set_id(1);
          invocation.set_method_name("");
          invocation.set_parameters(buffer, 0);
--        invocation.set_protocol_version(1);
++        invocation.set_protocol_version(mir::protobuf::current_protocol_version());
          invocation.set_side_channel_fds(2);
          auto const body_size = invocation.ByteSize();
          buffer[0] = body_size / 0x100;