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 for not throwing exception in cpv::RawReaderMemory when processin… #8594

Merged
merged 2 commits into from
Apr 15, 2022

Conversation

sevdokim
Copy link
Contributor

…g corrupted RDH
Hello @davidrohr @shahor02
This is a fix to catch exception which is thrown when reading corrupted cpv raw data.

@shahor02
Copy link
Collaborator

@sevdokim the

RawErrorType_t RawReaderMemory::next()
will now detect the error and return a proper error code (w/o rethrowing) but the caller function does not check the returned value, it just catches exceptions.
You should check it and in case of error abandon the TF processing (still sending output to not block the workflow and logging corresponding message, like you do for the deadbeef).

@sevdokim
Copy link
Contributor Author

Hi @shahor02
is it really necessary to abandon whole TF in case of bad rdh catched? Currently it just skips page with bad rdh and process the rest of TF.

@shahor02
Copy link
Collaborator

@sevdokim strictly speaking it is not necessary to skip whole TF, but RDH errors usually mean memory problems, likely the whole TF will be corrupted.
In any case, I don't see where do you skip the page with bad RDH: in

you are not checking the return code of the next() and proceed to decoding unless the exception was caught, but it will not be caught there since you have a catch in the nextPage().

@sevdokim
Copy link
Contributor Author

Hi @shahor02
you are absolutely right! Thank you very much for careful review of the code. Skipping of corrupted tf is implemented now. It seems to be working fine with randomly generated rdh versions.

@davidrohr davidrohr merged commit e2a942a into AliceO2Group:dev Apr 15, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants