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:
[email protected]
