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

Do not enforce API processing timeout for parallel routing #5114

Merged

Conversation

vpaturet
Copy link
Contributor

@vpaturet vpaturet commented May 12, 2023

Summary

As reported in #5110, deadlocks can occur when the parallel routing feature is activated. This is a regression caused by #5047:
To propagate the timeout to other threads, the initial ForkJoinPool/CompletableFuture logic used in parallel routing has been replaced by a fixed thread pool, because ForkJoinPool does not propagate interruptions.
But the work-stealing properties of the ForkJoinPool are necessary to prevent deadlocks.

This PR reverts to a ForkJoinPool implementation for the parallel routing use case.
This change does not impact API processing timeout when parallel routing is not in use.

Note: The parallel heuristic feature does not trigger deadlocks since it uses its own thread pool and does not create sub-tasks.

Issue

closes #5110

Unit tests

Documentation

Updated documentation

@codecov
Copy link

codecov bot commented May 12, 2023

Codecov Report

Patch coverage has no change and project coverage change: +0.03 🎉

Comparison is base (538ef07) 64.75% compared to head (af91e92) 64.78%.

Additional details and impacted files
@@              Coverage Diff              @@
##             dev-2.x    #5114      +/-   ##
=============================================
+ Coverage      64.75%   64.78%   +0.03%     
- Complexity     14123    14126       +3     
=============================================
  Files           1729     1728       -1     
  Lines          67369    67352      -17     
  Branches        7217     7215       -2     
=============================================
+ Hits           43623    43633      +10     
+ Misses         21324    21296      -28     
- Partials        2422     2423       +1     
Impacted Files Coverage Δ
...entripplanner/routing/algorithm/RoutingWorker.java 78.44% <0.00%> (ø)
.../algorithm/raptoradapter/router/TransitRouter.java 75.67% <0.00%> (ø)
...r/standalone/config/routerconfig/ServerConfig.java 100.00% <ø> (ø)
...ntripplanner/standalone/server/MetricsLogging.java 0.00% <ø> (ø)

... and 1 file with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@vpaturet vpaturet marked this pull request as ready for review May 12, 2023 10:34
@vpaturet vpaturet requested a review from a team as a code owner May 12, 2023 10:34
Copy link
Member

@leonardehrenfried leonardehrenfried left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is an acceptable solution to a hard problem, so I approve. We should probably discuss this a bit during the meeting.

@t2gran t2gran added this to the 2.4 (next release) milestone May 16, 2023
@t2gran t2gran merged commit fd9e022 into opentripplanner:dev-2.x May 22, 2023
5 checks passed
@t2gran t2gran deleted the fix_interruptible_executor_deadlock branch May 22, 2023 08:55
t2gran pushed a commit that referenced this pull request May 22, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants