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

Add execution timeout to Command Extractor #622

Merged
merged 1 commit into from
Jun 26, 2023
Merged

Add execution timeout to Command Extractor #622

merged 1 commit into from
Jun 26, 2023

Conversation

qkaiser
Copy link
Contributor

@qkaiser qkaiser commented Jun 23, 2023

Fix #621

@qkaiser qkaiser added the enhancement New feature or request label Jun 23, 2023
@qkaiser qkaiser self-assigned this Jun 23, 2023
@qkaiser qkaiser requested a review from e3krisztian June 26, 2023 07:23
Copy link
Contributor

@e3krisztian e3krisztian 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 it solves a concrete problem, so I do not request bigger immediate changes, but I am slightly unhappy about it:

  • 12 hours is too much for a single extractor, I would expect them to be fast, and would prefer a 10 minute limit instead, if that limit is not enough, we should definitely look at what is wrong - do we know of something slow, and non-problematic?
  • the limit is not configurable
  • it is in a need of a test (monkey-patching a sub-second timeout and sleep 60 as external command would work)
  • it is only for external commands?

@qkaiser
Copy link
Contributor Author

qkaiser commented Jun 26, 2023

I think it solves a concrete problem, so I do not request bigger immediate changes, but I am slightly unhappy about it:

  • 12 hours is too much for a single extractor, I would expect them to be fast, and would prefer a 10 minute limit instead, if that limit is not enough, we should definitely look at what is wrong - do we know of something slow, and non-problematic?

Extraction time is dependent on the input file size. We chose this value because the objective is to make sure unblob stops at some point, not to spot slow extractors.

  • the limit is not configurable

Yes, I don't think we want it to be at this point.

  • it is in a need of a test (monkey-patching a sub-second timeout and sleep 60 as external command would work)

I can add it to this branch, what do you say ?

  • it is only for external commands?

Yes, endless loops have been observed in external commands. When that happens in our own code we can fix it :)

@qkaiser qkaiser merged commit 5f2bd28 into main Jun 26, 2023
@qkaiser qkaiser deleted the 621-add-timeout branch June 26, 2023 13:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

No timeout defined for Command Extractor
2 participants