[Merge] lp:~mandel/location-service/handle-exceptions into lp:location-service

Jim Hodapp jim.hodapp at canonical.com
Tue Apr 14 17:58:38 UTC 2015


Review: Needs Fixing code

Looks good, just a few typos to fix.

Diff comments:

> === modified file 'src/location_service/com/ubuntu/location/connectivity/ofono_nm_connectivity_manager.cpp'
> --- src/location_service/com/ubuntu/location/connectivity/ofono_nm_connectivity_manager.cpp	2015-01-06 12:35:02 +0000
> +++ src/location_service/com/ubuntu/location/connectivity/ofono_nm_connectivity_manager.cpp	2015-04-14 14:29:11 +0000
> @@ -705,7 +705,24 @@
>  
>          worker_thread = std::move(std::thread
>          {
> -            [this]() { system_bus->run(); }
> +            [this]()
> +            {
> +                while(true) {
> +                    try
> +                    {
> +                        system_bus->run();
> +                        break;  // run exited normally
> +                    }
> +                    catch (const std::exception& e)
> +                    {
> +                        LOG(WARNING) << e.what();
> +                    }
> +                    catch(...)
> +                    {
> +                        LOG(WARNING) << "Unexpected exception was raised by the io executor.";
> +                    }
> +                }
> +            }
>          });
>      }
>  
> 
> === modified file 'src/location_service/com/ubuntu/location/connectivity/ofono_nm_connectivity_manager.h'
> --- src/location_service/com/ubuntu/location/connectivity/ofono_nm_connectivity_manager.h	2014-11-10 20:12:56 +0000
> +++ src/location_service/com/ubuntu/location/connectivity/ofono_nm_connectivity_manager.h	2015-04-14 14:29:11 +0000
> @@ -126,7 +126,25 @@
>              // And a dedicated worker thread.
>              std::thread worker
>              {
> -                [this]() { service.run(); }
> +                [this]() 
> +                {
> +                   while(true)
> +                   {
> +                      try
> +                      {
> +                          service.run();
> +                          break;  // normal exit
> +                      }
> +                      catch (const std::exception& e)
> +                      {
> +                          LOG(WARNING) << e.what();
> +                      }
> +                      catch(...)
> +                      {
> +                          LOG(WARNING) << "Received unexpected exception.";
> +                      }
> +                   }
> +                }
>              };
>          } dispatcher;
>  
> 
> === modified file 'src/location_service/com/ubuntu/location/providers/remote/provider.cpp'
> --- src/location_service/com/ubuntu/location/providers/remote/provider.cpp	2015-01-26 19:13:52 +0000
> +++ src/location_service/com/ubuntu/location/providers/remote/provider.cpp	2015-04-14 14:29:11 +0000
> @@ -66,6 +66,7 @@
>              try
>              {
>                  service.run();
> +                break;  // provider was excit correctly

Typo, "was excit" should be "has exited"

>              }
>              catch (const std::exception& e)
>              {
> 
> === modified file 'src/location_service/com/ubuntu/location/service/daemon.cpp'
> --- src/location_service/com/ubuntu/location/service/daemon.cpp	2015-02-13 13:19:18 +0000
> +++ src/location_service/com/ubuntu/location/service/daemon.cpp	2015-04-14 14:29:11 +0000
> @@ -15,6 +15,8 @@
>   *
>   * Authored by: Thomas Voß <thomas.voss at canonical.com>
>   */
> +
> +#include <com/ubuntu/location/logging.h>
>  #include <com/ubuntu/location/boost_ptree_settings.h>
>  #include <com/ubuntu/location/provider_factory.h>
>  
> @@ -239,11 +241,52 @@
>      };
>  
>      auto location_service = std::make_shared<location::service::Implementation>(configuration);
> -
> -    std::thread t1{[&config](){config.incoming->run();}};
> -    std::thread t2{[&config](){config.incoming->run();}};
> -    std::thread t3{[&config](){config.incoming->run();}};
> -    std::thread t4{[&config](){config.outgoing->run();}};
> +    // we need to ensure that is any exception is raised by the executor that we do not let it crash the app

Rephrase to: "We need to ensure that any exception raised by the executor does not crash the app and also gets logged."

> +    // and we log the issue
> +    auto incomingExecutorRunner = [&config] {
> +        while(true)
> +        {
> +            try
> +            {
> +                VLOG(10) << "Starting the incoming executor";
> +                config.incoming->run();
> +                break; // run() exited normally
> +            }
> +            catch (const std::exception& e)
> +            {
> +                LOG(WARNING) << e.what();
> +            }
> +            catch (...)
> +            {
> +                LOG(WARNING) << "Unexpected exceptions was raised by the incomming dbus executor";

Typo, should be "Unexpected exception was raised by the incoming executor"

> +            }
> +        }
> +    };
> +
> +    auto outgoingExecutorRunner = [&config] {
> +        while(true)
> +        {
> +            try
> +            {
> +                VLOG(10) << "Starting the outgoing executor";
> +                config.outgoing->run();
> +                break; // run() exited normally
> +            }
> +            catch (const std::exception& e)
> +            {
> +                LOG(WARNING) << e.what();
> +            }
> +            catch (...)
> +            {
> +                LOG(WARNING) << "Unexpected exceptions was raised by the outgoing dbus executor";
> +            }
> +        }
> +    };
> +
> +    std::thread t1{incomingExecutorRunner};
> +    std::thread t2{incomingExecutorRunner};
> +    std::thread t3{incomingExecutorRunner};
> +    std::thread t4{outgoingExecutorRunner};
>  
>      trap->run();
>  
> 


-- 
https://code.launchpad.net/~mandel/location-service/handle-exceptions/+merge/256157
Your team Ubuntu Phablet Team is subscribed to branch lp:location-service.



More information about the Ubuntu-reviews mailing list