Mir

Merge lp://staging/~robertcarr/mir/socket-messenger-reporting into lp://staging/mir

  1. socket-messenger-reporting
  2. Merge into development-branch
Proposed by Robert Carr
Status: Rejected
Rejected by: Robert Carr
Proposed branch: lp://staging/~robertcarr/mir/socket-messenger-reporting
Merge into: lp://staging/mir
Diff against target: 573 lines (+279/-13)
17 files modified
debian/changelog (+8/-2)
include/server/mir/default_server_configuration.h (+3/-0)
include/server/mir/frontend/connector_report.h (+0/-1)
include/server/mir/frontend/messenger_report.h (+49/-0)
include/server/mir/frontend/protobuf_ipc_factory.h (+3/-1)
include/server/mir/logging/messenger_report.h (+49/-0)
include/test/mir_test_doubles/stub_ipc_factory.h (+6/-1)
src/server/default_server_configuration.cpp (+31/-3)
src/server/frontend/CMakeLists.txt (+1/-0)
src/server/frontend/null_messenger_report.cpp (+25/-0)
src/server/frontend/protobuf_session_creator.cpp (+2/-2)
src/server/frontend/socket_messenger.cpp (+11/-2)
src/server/frontend/socket_messenger.h (+6/-1)
src/server/logging/CMakeLists.txt (+1/-0)
src/server/logging/messenger_report.cpp (+46/-0)
tests/unit-tests/frontend/CMakeLists.txt (+1/-0)
tests/unit-tests/frontend/test_socket_messenger.cpp (+37/-0)
To merge this branch: bzr merge lp://staging/~robertcarr/mir/socket-messenger-reporting
Reviewer Review Type Date Requested Status
Daniel van Vugt Needs Fixing
Alan Griffiths Disapprove
Kevin DuBois (community) Needs Fixing
PS Jenkins bot (community) continuous-integration Approve
Review via email: mp+187942@code.staging.launchpad.net

This proposal supersedes a proposal from 2013-09-26.

Commit message

Report socket messenger errors

Description of the change

Report socket messenger write errors so we can keep track of this.

Resubmitted to catch jenkins attention

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

[ try]
9 {
50 ba::write(*socket, ba::buffer(whole_message));
51 }
52 - catch (std::exception &)
53 + catch (std::exception &ex)
54 {
55 - // Don't care
56 + report->error(ex);
57 }

This is better written as:

    boost::system::error_code ec;
    ba::write(*socket, ba::buffer(whole_message), ec);

    if (!ec)
    {
        report->error(ec.message());
    }

And not throwing and catching exceptions.

review: Needs Fixing
Revision history for this message
Robert Carr (robertcarr) wrote : Posted in a previous version of this proposal

Fixed to use the error code, updating to trunk with the reorganization of some frontend interfaces (notably communicator_report->connector_report) made it necessary to introduce a new reporting interface. messenger_report

Revision history for this message
Robert Carr (robertcarr) wrote : Posted in a previous version of this proposal

Branch confusion....

Revision history for this message
Alan Griffiths (alan-griffiths) wrote : Posted in a previous version of this proposal

418 + if (!err)

Sorry, my typo - there should be no "!"! (Shouldn't a test detect this is wrong?)

And what is the deal with debian/changelog?

review: Needs Fixing
Revision history for this message
PS Jenkins bot (ps-jenkins) wrote : Posted in a previous version of this proposal

FAILED: Continuous integration, rev:1076
No commit message was specified in the merge proposal. Click on the following link and set the commit message (if you want a jenkins rebuild you need to trigger it yourself):
https://code.launchpad.net/~robertcarr/mir/socket-messenger-reporting/+merge/187822/+edit-commit-message

http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-ci/38/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-saucy-i386-build/2151
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-saucy-amd64-build/2036
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-saucy-amd64-ci/38
        deb: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-saucy-amd64-ci/38/artifact/work/output/*zip*/output.zip

Click here to trigger a rebuild:
http://s-jenkins:8080/job/mir-team-mir-development-branch-ci/38/rebuild

review: Needs Fixing (continuous-integration)
Revision history for this message
PS Jenkins bot (ps-jenkins) wrote :

PASSED: Continuous integration, rev:1077
http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-ci/46/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-saucy-i386-build/2160
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-saucy-amd64-build/2045
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-saucy-amd64-ci/46
        deb: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-saucy-amd64-ci/46/artifact/work/output/*zip*/output.zip

Click here to trigger a rebuild:
http://s-jenkins:8080/job/mir-team-mir-development-branch-ci/46/rebuild

review: Approve (continuous-integration)
Revision history for this message
Kevin DuBois (kdub) wrote :

nits:

needed?
499 +#include <sstream>
500 +#include <cstring>
501 +#include <mutex>

113 + void error(std::string const& error_message);
might be better as report_error(...)

review: Needs Fixing
Revision history for this message
Kevin DuBois (kdub) wrote :

i can't decide if we need a new report for this... seems if a message fails to send, the component that intended to send the message would care about knowing that. So, if there's an error sending, we could throw where we report an error in this MP... and the protobuf message processor could log that (it already has a logging class). the protobuf message processor could then rethrow and we'd clean up the resources for that client. Or, if the shell wanted to send a MirEvent, it could take appropriate action if that failed to send.

Revision history for this message
Alan Griffiths (alan-griffiths) wrote :

I think this is the wrong approach.

As Kevin says, we should be throwing an exception if we can't send a message. Not reporting it and ignoring the problem. It is the client code that should do any reporting.

This started off as a "suppress the error" as a temporary fix, but this is too much code churn for a temporary fix. Better to do it right.

review: Disapprove
Revision history for this message
Daniel van Vugt (vanvugt) wrote :

Aside from anything else, this proposal contains my own branch which today was rejected.

review: Needs Fixing

Unmerged revisions

1077. By Robert Carr

Merge development branch

1076. By Robert Carr

Update tests

1075. By Robert Carr

Merge trunk and update to new frontend interfaces

1074. By Robert Carr

Typo

1073. By Robert Carr

socket_messenger: Report exceptions

1072. By Daniel van Vugt

Stop asio::write from throwing exceptions (SIGPIPE). It will only do so
after we've closed the socket, at which point we don't really care.
(LP: #1226139)

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
The diff is not available at this time. You can reload the page or download it.

Subscribers

People subscribed via source and target branches