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

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

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

Conversation

jorgee
Copy link
Contributor

@jorgee jorgee commented Nov 5, 2024

close #5464

Remove errorStrategy check in getMaxRetries method. It avoids possible deadlock when using task.maxRetries in dynamic errorStrategy directives.

Default maxRetries is set to 1

…k when using task.maxRetries to set the errorStrategy

Signed-off-by: jorgee <[email protected]>
Copy link

netlify bot commented Nov 5, 2024

Deploy Preview for nextflow-docs-staging ready!

Name Link
🔨 Latest commit 36bca1e
🔍 Latest deploy log https://app.netlify.com/sites/nextflow-docs-staging/deploys/67321deeb502920008ad5da4
😎 Deploy Preview https://deploy-preview-5474--nextflow-docs-staging.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@pditommaso
Copy link
Member

A test is failing here

~ Test 'stub-retry.nf' run failed
   + set -e
   + echo ''
   
   + /home/runner/work/nextflow/nextflow/nextflow -q run ../../stub-retry.nf -stub
   + tee stdout
   ERROR ~ Error executing process > 'stubtest'
   
   Caused by:
     Missing output file(s) `*.txt` expected by process `stubtest`
   
   
   Command executed:
   
     echo "Stubbing. Not creating file"
   
   Command exit status:
     0
   
   Command output:
     Stubbing. Not creating file
   
   Work dir:
     /home/runner/work/nextflow/nextflow/tests/checks/scratch/92/85bf921c434bf3554b86f5eb4fa660
   
   Tip: you can replicate the issue by changing to the process work dir and entering the command `bash .command.run`
   
    -- Check '.nextflow.log' file for details
   ++ grep INFO .nextflow.log
   ++ grep -c 'Submitted process > stubtest'
   + [[ 1 == 1 ]]
   ++ grep INFO .nextflow.log
   ++ grep -c 'Re-submitted process > stubtest'
   + [[ 0 == 1 ]]
   + false

@jorgee jorgee marked this pull request as draft November 6, 2024 20:30
@jorgee jorgee closed this Nov 6, 2024
@jorgee jorgee reopened this Nov 6, 2024
@jorgee
Copy link
Contributor Author

jorgee commented Nov 6, 2024

Yes, It is failing because maxRetries is 0. I need to check why it is returning 0 instead of the default one. I have converted to draft

@jorgee jorgee marked this pull request as ready for review November 7, 2024 13:15
@jorgee
Copy link
Contributor Author

jorgee commented Nov 11, 2024

A test is failing here

~ Test 'stub-retry.nf' run failed
   + set -e
   + echo ''
   
   + /home/runner/work/nextflow/nextflow/nextflow -q run ../../stub-retry.nf -stub
   + tee stdout
   ERROR ~ Error executing process > 'stubtest'
   
   Caused by:
     Missing output file(s) `*.txt` expected by process `stubtest`
   
   
   Command executed:
   
     echo "Stubbing. Not creating file"
   
   Command exit status:
     0
   
   Command output:
     Stubbing. Not creating file
   
   Work dir:
     /home/runner/work/nextflow/nextflow/tests/checks/scratch/92/85bf921c434bf3554b86f5eb4fa660
   
   Tip: you can replicate the issue by changing to the process work dir and entering the command `bash .command.run`
   
    -- Check '.nextflow.log' file for details
   ++ grep INFO .nextflow.log
   ++ grep -c 'Submitted process > stubtest'
   + [[ 1 == 1 ]]
   ++ grep INFO .nextflow.log
   ++ grep -c 'Re-submitted process > stubtest'
   + [[ 0 == 1 ]]
   + false

Tests are fixed

@@ -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

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.

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.

Setting default maxRetries and errorStrategy and override for some processes
2 participants