Difference between revisions of "Error Handling with the Single Input Single Output Pattern"
(→GoTo Disclaimers) |
m |
||
Line 7: | Line 7: | ||
Despite this, it is quite controversial among de developer's community due to the use of the infamous '''goto''' operator. The idea in ''single-input single-output'' is simple: | Despite this, it is quite controversial among de developer's community due to the use of the infamous '''goto''' operator. The idea in ''single-input single-output'' is simple: | ||
− | + | Every function should have a single entry point and a single output point. | |
The single entry point is natural for most of us, and probably (hopefully) de 100% of the cases. It is breakable though. In some old non-standard C compilers you could jump (using a '''goto''') to a line in another function. Other more day-to-day examples include the use of '''setjump''' or '''longjump'''. | The single entry point is natural for most of us, and probably (hopefully) de 100% of the cases. It is breakable though. In some old non-standard C compilers you could jump (using a '''goto''') to a line in another function. Other more day-to-day examples include the use of '''setjump''' or '''longjump'''. | ||
Line 22: | Line 22: | ||
* Yes, you can achieve the same with nested if/else structures. No, it won't be as readable and clean. | * Yes, you can achieve the same with nested if/else structures. No, it won't be as readable and clean. | ||
− | + | {{Ambox | |
− | No, you shouldn't be applying this to C++. Instead, you should be using any form of RAII like smart pointers, for example. | + | |type=notice |
− | + | |small=left | |
+ | |issue=No, you shouldn't be applying this to C++. Instead, you should be using any form of RAII like smart pointers, for example. | ||
+ | |style=width:unset; | ||
+ | }} | ||
== Applying the Pattern == | == Applying the Pattern == |
Revision as of 10:25, 18 December 2021
Contents
Introduction
The single-input single-output is a simple pattern for procedural C code that eases:
- Graceful error handling
- Avoid code duplication
- Improve readability.
Despite this, it is quite controversial among de developer's community due to the use of the infamous goto operator. The idea in single-input single-output is simple: Every function should have a single entry point and a single output point.
The single entry point is natural for most of us, and probably (hopefully) de 100% of the cases. It is breakable though. In some old non-standard C compilers you could jump (using a goto) to a line in another function. Other more day-to-day examples include the use of setjump or longjump.
The single output point is way more common but gladly less harmful. The simplest example being having multiple returns in a single function. This is very typical during error handling or short-circuiting based on certain conditions.
GoTo Disclaimers
First of all, this is my personal opinion. Having said that:
- I do not encourage the arbitrary use of goto jumps.
- The single-input single-output is one of the few cases in which I believe is acceptable, even beneficial.
- Yes, you can achieve the same with nested if/else structures. No, it won't be as readable and clean.
No, you shouldn't be applying this to C++. Instead, you should be using any form of RAII like smart pointers, for example. |
Applying the Pattern
Consider the following excerpt from a code I just recently reviewed. I've simplified it for readability. This code has, deliberately, no error handling. This is what we are trying to solve.
1 static GstFlowReturn
2 gst_example_extract_raw_buffer (GstNvMediaDemux * demux,
3 GstBuffer * buffer, GstBuffer ** raw_buffer)
4 {
5 gint nvstatus = NVMEDIA_STATUS_OK;
6 GstFlowReturn ret = GST_FLOW_OK;
7 guint bytes_per_pixel = 2;
8 NvMediaImageSurfaceMap surface_map = { 0 };
9 NvMediaImage *image = NULL;
10 GstNvMediaMeta *meta = NULL;
11 guint8 *data = NULL;
12
13 g_return_val_if_fail (demux, GST_FLOW_ERROR);
14 g_return_val_if_fail (buffer, GST_FLOW_ERROR);
15 g_return_val_if_fail (raw_buffer, GST_FLOW_ERROR);
16
17 *raw_buffer = NULL;
18
19 meta = gst_buffer_get_nv_media_meta (buffer);
20
21 image = meta->image_raw;
22 nvstatus = NvMediaImageLock (image, NVMEDIA_IMAGE_ACCESS_READ,
23 &surface_map);
24
25 image_size = gst_example_compute_image_size (&surface_map);
26 data = (guint8 *) g_malloc (image_size * sizeof (guint8));
27
28 nvstatus = NvMediaImageGetBits (image, data, image_size);
29
30 *raw_buffer = gst_buffer_new_wrapped (buff, image_size);
31
32 gst_example_analyze (*raw_buffer);
33
34 NvMediaImageUnlock (meta->image_raw);
35
36 return ret;
37 }
Error Handling
This code has to error handling at all. Let's add some, shall we? One first approach can be thought of as the following:
1 static GstFlowReturn
2 gst_example_extract_raw_buffer (GstNvMediaDemux * demux,
3 GstBuffer * buffer, GstBuffer ** raw_buffer)
4 {
5 /* Declarations and pointer guards */
6
7 meta = gst_buffer_get_nv_media_meta (buffer);
8 if (NULL == meta) {
9 GST_ERROR_OBJECT (demux, "No NvMedia meta found, is the and NvMedia buffer?");
10 return GST_FLOW_ERROR;
11 }
12
13 image = meta->image_raw;
14 nvstatus = NvMediaImageLock (image, NVMEDIA_IMAGE_ACCESS_READ,
15 &surface_map);
16 if (NVMEDIA_STATUS_OK != nvstatus) {
17 GST_ERROR_OBJECT (demux, "Unable to lock NvMedia image: %d", nvstatus);
18 return GST_FLOW_ERROR;
19 }
20
21 image_size = gst_example_compute_image_size (&surface_map);
22 data = (guint8 *) g_malloc (image_size * sizeof (guint8));
23 if (NULL == data) {
24 GST_ERROR_OBJECT (demux, "Unable to alloc buffer, system ran out of memory");
25 return GST_FLOW_ERROR;
26 }
27
28 nvstatus = NvMediaImageGetBits (image, data, image_size);
29 if (NVMEDIA_STATUS_OK != nvstatus) {
30 GST_ERROR_OBJECT (demux, "Unable to lock NvMedia image: %d", nvstatus);
31 return GST_FLOW_ERROR;
32 }
33
34 *raw_buffer = gst_buffer_new_wrapped (buff, image_size);
35 if (NULL == *raw_buffer) {
36 GST_ERROR_OBJECT (demux, "Unable to wrap data, system ran out of memory");
37 return GST_FLOW_ERROR;
38 }
39
40 if (FALSE == gst_example_analyze (*raw_buffer)) {
41 GST_ERROR_OBJECT (demux, "Some business logic error");
42 return GST_FLOW_ERROR;
43 }
44
45 NvMediaImageUnlock (meta->image_raw);
46
47 return ret;
48 }
This is the classical single input multiple output' scenarios. Hope you noticed the horrors in this approach :)[1]
Freeing Resources
Let's deal with the forgotten resources.
1 static GstFlowReturn
2 gst_example_extract_raw_buffer (GstNvMediaDemux * demux,
3 GstBuffer * buffer, GstBuffer ** raw_buffer)
4 {
5 /* Declarations and pointer guards */
6
7 meta = gst_buffer_get_nv_media_meta (buffer);
8 if (NULL == meta) {
9 GST_ERROR_OBJECT (demux, "No NvMedia meta found, is the and NvMedia buffer?");
10 return GST_FLOW_ERROR;
11 }
12
13 image = meta->image_raw;
14 nvstatus = NvMediaImageLock (image, NVMEDIA_IMAGE_ACCESS_READ,
15 &surface_map);
16 if (NVMEDIA_STATUS_OK != nvstatus) {
17 GST_ERROR_OBJECT (demux, "Unable to lock NvMedia image: %d", nvstatus);
18 return GST_FLOW_ERROR;
19 }
20
21 image_size = gst_example_compute_image_size (&surface_map);
22 data = (guint8 *) g_malloc (image_size * sizeof (guint8));
23 if (NULL == data) {
24 GST_ERROR_OBJECT (demux, "Unable to alloc buffer, system ran out of memory");
25 NvMediaImageUnlock (meta->image_raw); /* <---- Freeing a resource */
26 return GST_FLOW_ERROR;
27 }
28
29 nvstatus = NvMediaImageGetBits (image, data, image_size);
30 if (NVMEDIA_STATUS_OK != nvstatus) {
31 GST_ERROR_OBJECT (demux, "Unable to lock NvMedia image: %d", nvstatus);
32 NvMediaImageUnlock (meta->image_raw); /* <---- Freeing a resource */
33 g_free (data); /* <--------------------------- Freeing a resource */
34 return GST_FLOW_ERROR;
35 }
36
37 *raw_buffer = gst_buffer_new_wrapped (buff, image_size);
38 if (NULL == *raw_buffer) {
39 GST_ERROR_OBJECT (demux, "Unable to wrap data, system ran out of memory");
40 NvMediaImageUnlock (meta->image_raw); /* <---- Freeing a resource */
41 g_free (data); /* <--------------------------- Freeing a resource */
42 return GST_FLOW_ERROR;
43 }
44
45 if (FALSE == gst_example_analyze (*raw_buffer)) {
46 GST_ERROR_OBJECT (demux, "Some business logic error");
47 NvMediaImageUnlock (meta->image_raw); /* <---- Freeing a resource */
48 gst_buffer_unref (*raw_buf); /* <------------- Freeing a resource */
49 return GST_FLOW_ERROR;
50 }
51
52 NvMediaImageUnlock (meta->image_raw);
53
54 return ret;
55 }
Do you see how the error handling starts to pile up with duplicate code?
Refactoring to SISO
First, let's analyze the resources acquisition and their order:
- ↑ No resources are freed here, leading to leaks and potential deadlocks!