celix-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From GitBox <...@apache.org>
Subject [GitHub] [celix] Oipo commented on a change in pull request #316: Initial executor abstraction to promise
Date Mon, 01 Feb 2021 17:21:55 GMT

Oipo commented on a change in pull request #316:
URL: https://github.com/apache/celix/pull/316#discussion_r567963386



##########
File path: misc/experimental/promise/README.md
##########
@@ -96,25 +84,18 @@ void processPayload(celix::Promise<std::shared_ptr<RestApi::Payload>>
promise) {
 }
 ```
 
-## Open Issues & TODOs
-
-### Circular References in the execution model
+## Differences with OSGi Promises & Java
 
-There is a issue with circular references of shared_ptr between the executor and
-SharedPromiseState. SharedPromiseState has a shared_ptr to the executor to delegate 
-execution of chains (onResolve, onSuccess, etc) and the executor accept 
-SharedPromiseState - in the form of tasks - to really execute those chains. 
-This will lead to the situation where the process exists, but the destructor of 
-the executor any some SharedPromiseState are not called leading to mem leaks.
-  
-<b>TODO Discuss</b>: One way to solve this is that the SharedPromiseState has
a weak_ptr to the 
-executor and only delegates chain execution if the executor still exists. 
-If the executor is gone the chains will not be executor resulting into a error situation.

+1. There is no singleton default executor. A PromiseFactory can be construction argument-less
to create a default executor, but this executor is then bound to the lifecycle of the PromiseFactory.
If celix::IExecutor is injected in the PromiseFactory, it is up to user to control the complete
lifecycle of the executor (e.g. by providing this in e ThreadExecutionModel bundle and ensuring
this is started early (and as result stopped late).

Review comment:
       `e.g. by providing this in **e** ThreadExecutionModel` -> `e.g. by providing this
in **a** ThreadExecutionModel`

##########
File path: misc/experimental/promise/api/celix/RejectedExecutionException.h
##########
@@ -0,0 +1,32 @@
+/**
+ *Licensed to the Apache Software Foundation (ASF) under one
+ *or more contributor license agreements.  See the NOTICE file
+ *distributed with this work for additional information
+ *regarding copyright ownership.  The ASF licenses this file
+ *to you under the Apache License, Version 2.0 (the
+ *"License"); you may not use this file except in compliance
+ *with the License.  You may obtain a copy of the License at
+ *
+ *  http://www.apache.org/licenses/LICENSE-2.0
+ *
+ *Unless required by applicable law or agreed to in writing,
+ *software distributed under the License is distributed on an
+ *"AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied.  See the License for the
+ *specific language governing permissions and limitations
+ *under the License.
+ */
+
+#pragma once
+
+namespace celix {
+
+    struct RejectedExecutionException  : public std::exception
+    {
+        [[nodiscard]] const char * what () const throw ()

Review comment:
       ["throw() is equivalent to noexcept(true) but was deprecated with C++11 and will be
removed with C++20. "](https://www.modernescpp.com/index.php/c-core-guidelines-the-noexcept-specifier-and-operator)

##########
File path: misc/experimental/promise/README.md
##########
@@ -96,25 +84,18 @@ void processPayload(celix::Promise<std::shared_ptr<RestApi::Payload>>
promise) {
 }
 ```
 
-## Open Issues & TODOs
-
-### Circular References in the execution model
+## Differences with OSGi Promises & Java
 
-There is a issue with circular references of shared_ptr between the executor and
-SharedPromiseState. SharedPromiseState has a shared_ptr to the executor to delegate 
-execution of chains (onResolve, onSuccess, etc) and the executor accept 
-SharedPromiseState - in the form of tasks - to really execute those chains. 
-This will lead to the situation where the process exists, but the destructor of 
-the executor any some SharedPromiseState are not called leading to mem leaks.
-  
-<b>TODO Discuss</b>: One way to solve this is that the SharedPromiseState has
a weak_ptr to the 
-executor and only delegates chain execution if the executor still exists. 
-If the executor is gone the chains will not be executor resulting into a error situation.

+1. There is no singleton default executor. A PromiseFactory can be construction argument-less
to create a default executor, but this executor is then bound to the lifecycle of the PromiseFactory.
If celix::IExecutor is injected in the PromiseFactory, it is up to user to control the complete
lifecycle of the executor (e.g. by providing this in e ThreadExecutionModel bundle and ensuring
this is started early (and as result stopped late).

Review comment:
       `construction` -> `constructed`

##########
File path: misc/experimental/promise/api/celix/impl/SharedPromiseState.h
##########
@@ -208,12 +224,36 @@ namespace celix::impl {
 *********************************************************************************/
 
 template<typename T>
-inline celix::impl::SharedPromiseState<T>::SharedPromiseState(const tbb::task_arena&
_executor) : executor{_executor} {}
+std::shared_ptr<celix::impl::SharedPromiseState<T>> celix::impl::SharedPromiseState<T>::create(std::shared_ptr<celix::IExecutor>
_executor, int priority) {
+    auto state = std::shared_ptr<celix::impl::SharedPromiseState<T>>{new celix::impl::SharedPromiseState<T>{std::move(_executor),
priority}};
+    std::weak_ptr<celix::impl::SharedPromiseState<T>> self = state;
+    state->setSelf(std::move(self));

Review comment:
       [In c++17 `state->setSelf(state);` should work. ](https://en.cppreference.com/w/cpp/language/class_template_argument_deduction)
Otherwise, try `state->setSelf(std::weak_ptr(state));`.




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



Mime
View raw message