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

Catch resize exception and release client lock. #333

Merged
merged 2 commits into from Jun 28, 2023
Merged

Conversation

jrobble
Copy link
Member

@jrobble jrobble commented Jun 27, 2023

@jrobble jrobble self-assigned this Jun 27, 2023
@jrobble jrobble added this to To do in OpenMPF: Development via automation Jun 27, 2023
Copy link
Member

@brosenberg42 brosenberg42 left a comment

Choose a reason for hiding this comment

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

We should be throwing MPFDetectionException(MPF_BAD_FRAME_SIZE, "<explanation>"). We can check the dimensions and throw before calling cv::resize. An other option is to put a try block around just the cv::resize call, catch cv::Exception, and throw the mpf exception.

Reviewable status: 0 of 1 files reviewed, 1 unresolved discussion (waiting on @jrobble)


cpp/OcvYoloDetection/triton/TritonInferencer.cpp line 265 at r1 (raw file):

            }
        } 
        catch (const std::exception &ex) {

In addition to my comments above, change this to:

catch (...) {
    releaseClientId(clientId, nullptr);
    throw;
}

We catch ... because we never want to be deadlocked. Ideally, we would be calling releaseClientId in a destructor for a ClientLease object, but that is a bigger change.

std::current_exception() is for when you want access to an exception outside of its catch block. It is usually used to move an exception between threads.

Copy link
Member Author

@jrobble jrobble left a comment

Choose a reason for hiding this comment

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

Reviewable status: 0 of 4 files reviewed, 1 unresolved discussion (waiting on @brosenberg42 and @jrobble)


cpp/OcvYoloDetection/triton/TritonInferencer.cpp line 265 at r1 (raw file):

Previously, brosenberg42 wrote…

In addition to my comments above, change this to:

catch (...) {
    releaseClientId(clientId, nullptr);
    throw;
}

We catch ... because we never want to be deadlocked. Ideally, we would be calling releaseClientId in a destructor for a ClientLease object, but that is a bigger change.

std::current_exception() is for when you want access to an exception outside of its catch block. It is usually used to move an exception between threads.

I did it a different way, as discussed.

Copy link
Member

@brosenberg42 brosenberg42 left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewable status: 0 of 5 files reviewed, all discussions resolved (waiting on @jrobble)

Copy link
Member Author

@jrobble jrobble left a comment

Choose a reason for hiding this comment

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

Reviewed 3 of 3 files at r2, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @jrobble)

@jrobble jrobble merged commit 388b0d5 into master Jun 28, 2023
2 checks passed
@jrobble jrobble deleted the hf/yolo-resize branch June 28, 2023 17:16
OpenMPF: Development automation moved this from To do to Closed Jun 28, 2023
@jrobble jrobble mentioned this pull request Jun 28, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Closed
Development

Successfully merging this pull request may close these issues.

None yet

2 participants