Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Catch exception in lightweight node #623

Merged
merged 17 commits into from
Dec 8, 2021

Conversation

Iliamish
Copy link
Contributor

@Iliamish Iliamish commented Oct 19, 2021

Description

If user code throws exception inside body of lightweight node, that deadlock graph. In this patch we require from user mark body noexcept for using lightweight policy. If body can throw exception, policy implicitly implements queuing behavior.

Fixes # - #420

Type of change

Choose one or multiple, leave empty if none of the other choices apply

Add respective label(s) to PR if you have permissions

  • bug fix - change which fixes an issue
  • new feature - change which adds functionality
  • tests - change in tests
  • infrastructure - change in infrastructure and CI
  • documentation - documentation update

Tests

  • added - required for new features and for some bug fixes
  • not needed

Documentation

  • updated in # - add PR number
  • needs to be updated
  • not needed

Breaks backward compatibility

  • Yes
  • No
  • Unknown

Notify the following users

List users with @ to send notifications

Other information

@Iliamish Iliamish self-assigned this Oct 19, 2021
@Iliamish Iliamish changed the title Catch exception in lightweight node WIP: Catch exception in lightweight node Oct 19, 2021
test/tbb/test_flow_graph_priorities.cpp Show resolved Hide resolved
include/oneapi/tbb/flow_graph.h Show resolved Hide resolved
include/oneapi/tbb/detail/_flow_graph_node_impl.h Outdated Show resolved Hide resolved
test/tbb/test_multifunction_node.cpp Outdated Show resolved Hide resolved
test/tbb/test_multifunction_node.cpp Outdated Show resolved Hide resolved
test/tbb/test_flow_graph_priorities.cpp Show resolved Hide resolved
test/common/graph_utils.h Outdated Show resolved Hide resolved
include/oneapi/tbb/detail/_flow_graph_node_impl.h Outdated Show resolved Hide resolved
test/tbb/test_multifunction_node.cpp Outdated Show resolved Hide resolved
Signed-off-by: Mishin, Ilya <[email protected]>
Signed-off-by: Mishin, Ilya <[email protected]>
Signed-off-by: Mishin, Ilya <[email protected]>
Signed-off-by: Mishin, Ilya <[email protected]>
Signed-off-by: Mishin, Ilya <[email protected]>
Signed-off-by: Mishin, Ilya <[email protected]>
Signed-off-by: Mishin, Ilya <[email protected]>
Signed-off-by: Mishin, Ilya <[email protected]>
Signed-off-by: Mishin, Ilya <[email protected]>
Signed-off-by: Mishin, Ilya <[email protected]>
Signed-off-by: Mishin, Ilya <[email protected]>
Signed-off-by: Mishin, Ilya <[email protected]>
Signed-off-by: Mishin, Ilya <[email protected]>
@Iliamish Iliamish force-pushed the dev/iliamish/fix_exception_in_lightweight_policy branch from c0445be to 80d0385 Compare November 10, 2021 18:37
Signed-off-by: Mishin, Ilya <[email protected]>
test/common/graph_utils.h Outdated Show resolved Hide resolved
test/common/graph_utils.h Outdated Show resolved Hide resolved
test/tbb/test_flow_graph_priorities.cpp Show resolved Hide resolved
@Iliamish Iliamish changed the title WIP: Catch exception in lightweight node Catch exception in lightweight node Nov 12, 2021
@vlserov vlserov merged commit 324afd9 into master Dec 8, 2021
@vlserov vlserov deleted the dev/iliamish/fix_exception_in_lightweight_policy branch December 8, 2021 08:46
kboyarinov pushed a commit that referenced this pull request Dec 27, 2021
* Add raii to aggregator to prevent stuck

Signed-off-by: Mishin, Ilya <[email protected]>

* Add test

Signed-off-by: Mishin, Ilya <[email protected]>

* Reuse tbb raii guard

Signed-off-by: Mishin, Ilya <[email protected]>

* Add header

Signed-off-by: Mishin, Ilya <[email protected]>

* execute_and_wait lightweight task

Signed-off-by: Mishin, Ilya <[email protected]>

* remove unused headers

Signed-off-by: Mishin, Ilya <[email protected]>

* remove space

Signed-off-by: Mishin, Ilya <[email protected]>

* remove std::function

Signed-off-by: Mishin, Ilya <[email protected]>

* noexcept lightweight body

Signed-off-by: Mishin, Ilya <[email protected]>

* fix cmake

Signed-off-by: Mishin, Ilya <[email protected]>

* fix space

Signed-off-by: Mishin, Ilya <[email protected]>

* gcc error

Signed-off-by: Mishin, Ilya <[email protected]>

* declval gateway_type

Signed-off-by: Mishin, Ilya <[email protected]>

* new tests

Signed-off-by: Mishin, Ilya <[email protected]>

* template tests

Signed-off-by: Mishin, Ilya <[email protected]>

* macro TBB_USE_EXCEPTIONS

Signed-off-by: Mishin, Ilya <[email protected]>

* Apply suggestions from code review

Co-authored-by: Aleksei Fedotov <[email protected]>

Co-authored-by: Aleksei Fedotov <[email protected]>
@akukanov
Copy link

we require from user mark body noexcept for using lightweight policy. If body can throw exception, policy implicitly implements queuing behavior.

Is it requirement reflected in the oneTBB specification? I think it should.
@alexey-katranov @aleksei-fedotov

@aleksei-fedotov
Copy link
Contributor

we require from user mark body noexcept for using lightweight policy. If body can throw exception, policy implicitly implements queuing behavior.

Is it requirement reflected in the oneTBB specification? I think it should. @alexey-katranov @aleksei-fedotov

PR regarding the reflection: uxlfoundation/oneAPI-spec#407. @akukanov @alexey-katranov

@akukanov
Copy link

akukanov commented Feb 8, 2022

Another concern is that, though backward compatibility is not formally broken, the behavior of codes that use lightweight policies may implicitly change, resulting in a possible performance loss. @alexey-katranov, @aleksei-fedotov, have you considered some form of a warning to be issued when lightweight policies are applied without noexcept?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants