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

Fix race-condition reading job module details files #4948

Merged

Conversation

okurz
Copy link
Member

@okurz okurz commented Dec 7, 2022

A not readable test module result file is handled and gracefully
returned to the web UI and no error in log. The handling should be
atomic, not "check if readable and then read". Instead of trying to read
and then fail we should handle the read attempt as we handle the
exception in line 96 and return {} in case of error.

Related progress issue: https://progress.opensuse.org/issues/121444

@okurz okurz marked this pull request as draft December 7, 2022 10:51
Copy link
Contributor

@perlpunk perlpunk left a comment

Choose a reason for hiding this comment

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

I thought the conclusion in our meeting was that we do not want an exception if the file is not there (return {})

@okurz
Copy link
Member Author

okurz commented Dec 7, 2022

I thought the conclusion in our meeting was that we do not want an exception if the file is not there (return {})

I wanted to see test results and based on those see if maybe the exceptions are already handled on a higher level and turned into properly returned error messages

@asdil12
Copy link
Member

asdil12 commented Dec 7, 2022

I thought the conclusion in our meeting was that we do not want an exception if the file is not there (return {})

I think the idea is that we do want to use exceptions to handle that error because they don't introduce race conditions where we can still run into the issue, but catch those exceptions and return something like 404 to the client.

A not readable test module result file is handled and gracefully
returned to the web UI and no error in log. The handling should be
atomic, not "check if readable and then read". Instead of trying to read
and then fail we should handle the read attempt as we handle the
exception in line 96 and return {} in case of error.

Related progress issue: https://progress.opensuse.org/issues/121444
@okurz okurz force-pushed the feature/handle_json_read_error_121444 branch from e4e3434 to f05a288 Compare December 8, 2022 18:53
@okurz okurz marked this pull request as ready for review December 8, 2022 19:13
@okurz
Copy link
Member Author

okurz commented Dec 8, 2022

Updated the PR to explicitly return {} as originally discussed. This also means that tests pass again as the same path is taken as if the complete result directory is not readable anymore.

@mergify mergify bot merged commit 6b28620 into os-autoinst:master Dec 9, 2022
@okurz okurz deleted the feature/handle_json_read_error_121444 branch December 9, 2022 08:07
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.

5 participants