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.

Sign up for GitHub

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

Jump to bottom

Remove errorStrategy check in getMaxRetries to avoid possible deadloc… #5474

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

Remove errorStrategy check in getMaxRetries to avoid possible deadloc… #5474

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
a18b1ef
Remove errorStrategy check in getMaxRetries to avoid possible deadloc…
jorgee Nov 5, 2024
4cf5f4a
fix integration tests
jorgee Nov 7, 2024
6e3969e
fix process config test
jorgee Nov 7, 2024
36bca1e
Update modules/nextflow/src/main/groovy/nextflow/script/ProcessConfig…
pditommaso Nov 11, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 1 addition & 2 deletions modules/nextflow/src/main/groovy/nextflow/processor/TaskConfig.groovy
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -345,8 +345,7 @@ class TaskConfig extends LazyMap implements Cloneable {

int getMaxRetries() {
def result = get('maxRetries')
def defResult = getErrorStrategy() == ErrorStrategy.RETRY ? 1 : 0
result ? result as int : defResult
result != null ? result as int : 1
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
result != null ? result as int : 1
result ? result as int : 1

Better to keep as it was before

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I changed because result can be an integer and 0 is considered as false. It was breaking the test when number of retries is set to 0.

Copy link
Member

@pditommaso pditommaso Nov 11, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Why, as a user, should I use 0 retries with a retry error strategy?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think it has not too much sense but there was this possibility and it was in the test. Or we keep null check or we change the test.

}

int getMaxErrors() {
Expand Down
2 changes: 1 addition & 1 deletion modules/nextflow/src/main/groovy/nextflow/script/ProcessConfig.groovy
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -126,7 +126,7 @@ class ProcessConfig implements Map<String,Object>, Cloneable {
debug: false,
cacheable: true,
shell: BashWrapperBuilder.BASH,
maxRetries: 0,
maxRetries: 1,
maxErrors: -1,
errorStrategy: ErrorStrategy.TERMINATE
]
Expand Down
6 changes: 3 additions & 3 deletions modules/nextflow/src/test/groovy/nextflow/processor/TaskConfigTest.groovy
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -157,7 +157,7 @@ class TaskConfigTest extends Specification {

where:
value | expected
null | 0
null | 1
0 | 0
1 | 1
'3' | 3
Expand All @@ -171,8 +171,8 @@ class TaskConfigTest extends Specification {
when:
config = new TaskConfig()
then:
config.maxRetries == 0
config.getMaxRetries() == 0
config.maxRetries == 1
config.getMaxRetries() == 1
config.getErrorStrategy() == ErrorStrategy.TERMINATE

when:
Expand Down
2 changes: 1 addition & 1 deletion modules/nextflow/src/test/groovy/nextflow/script/ProcessConfigTest.groovy
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,7 @@ class ProcessConfigTest extends Specification {
expect:
config.shell == ['/bin/bash','-ue']
config.cacheable
config.maxRetries == 0
config.maxRetries == 1
config.maxErrors == -1
config.errorStrategy == ErrorStrategy.TERMINATE
}
Expand Down