From 0d11361820df25f8c611411137d1f0482b8372e6 Mon Sep 17 00:00:00 2001 From: ncteisen Date: Fri, 17 Mar 2017 14:33:45 -0500 Subject: [PATCH 1/4] Add error ownership doc --- doc/core/error-ownership-rules.md | 117 ++++++++++++++++++++++++++++++ 1 file changed, 117 insertions(+) create mode 100644 doc/core/error-ownership-rules.md diff --git a/doc/core/error-ownership-rules.md b/doc/core/error-ownership-rules.md new file mode 100644 index 00000000000..a8ae072ae02 --- /dev/null +++ b/doc/core/error-ownership-rules.md @@ -0,0 +1,117 @@ +gRPC Error Ownership Rules +===================== + +## Background + +`grpc_error` is the c-core's opaque representation of an error. It holds a collection of integers, strings, timestamps, and child errors that related to the final error. + +grpc_errors are refcounted objects, which means they need strict ownership semantics. An extra ref on an error can cause a memory leak, and a missing ref can cause a crash. + +This document serves as a detailed overview of grpc_error's ownership rules. It should help people use the errors, as well as help people debug refcount related errors. + +## Clarification of Ownership + +If a particular function is said to "own" an error, that means it has the responsibility of calling unref on the error. A function may have access to an error without ownership of it. + +This means the function may use the error, but must not call unref on it, since that will be done elsewhere in the code. A function that does not own an error may explicitly take ownership of it by manually calling GRPC_ERROR_REF. + +## Ownership Rules + +There are three rules of error ownership, which we will go over in detail. +* If `grpc_error` is returned by a function, the caller owns a ref to that instance. +* If a `grpc_error` is passed to a `grpc_closure` callback function, then that function does not own a ref to the error. +* if a `grpc_error` is passed to *any other function*, then that function takes ownership of the error. + +### Rule 1 + +> If `grpc_error` is returned by a function, the caller owns a ref to that instance.* + +For example, in the following code block, error1 and error2 are owned by the current function. + +```C +grpc_error* error1 = GRPC_ERROR_CREATE("Some error occured"); +grpc_error* error2 = some_operation_that_might_fail(...); +``` + +The current function would have to explicitly call GRPC_ERROR_UNREF on the errors, or pass them along to a function that would take over the ownership. + +### Rule 2 + +> If a `grpc_error` is passed to a `grpc_closure` callback function, then that function does not own a ref to the error. + +A `grpc_closure` callback function is any function that has the signature: + +```C +void (*cb)(grpc_exec_ctx *exec_ctx, void *arg, grpc_error *error); +``` + +This means that the error ownership is NOT transferred when a functions calls: + +```C +c->cb(exec_ctx, c->cb_arg, err); +``` + +The caller is still responsible for unref-ing the error. + +However, the above line is currently being phased out! It is safer to invoke callbacks with `grpc_closure_run` and `grpc_closure_sched`. These functions are not callbacks, so they will take ownership of the error passed to them. + +```C +grpc_error* error = GRPC_ERROR_CREATE("Some error occured"); +grpc_closure_run(exec_ctx, cb, error); +// current function no longer has ownership of the error +``` + +If you schedule or run a closure, but still need ownership of the error, then you must explicitly take a reference. + +```C +grpc_error* error = GRPC_ERROR_CREATE("Some error occured"); +grpc_closure_run(exec_ctx, cb, GRPC_ERROR_REF(error)); +// do some other things with the error +GRPC_ERROR_UNREF(error); +``` + +Rule 2 is more important to keep in mind when **implementing** `grpc_closure` callback functions. You must keep in mind that you do not own the error, and must not unref it. More importantly, you cannot pass it to any function that would take ownership of the error, without explicitly taking ownership yourself. For example: + +```C +void on_some_action(grpc_exec_ctx *exec_ctx, void *arg, grpc_error *error) { + // this would cause a crash, because some_function will unref the error, + // and the caller of this callback will also unref it. + some_function(error); + + // this callback function must take ownership, so it can give that + // ownership to the function it is calling. + some_function(GRPC_ERROR_REF(error)); +} +``` + +### Rule 3 + +> if a `grpc_error` is passed to *any other function*, then that function takes ownership of the error. + +Take the following example: + +```C +grpc_error* error = GRPC_ERROR_CREATE("Some error occured"); +// do some things +some_function(error); +// can't use error anymore! might be gone. +``` + +When some_function is called, it takes over the ownership of the error, and it will eventually unref it. So the caller can no longer safely use the error. + +If the caller needed to keep using the error (or passing it to other functions), if would have to take on a reference to it. This is a common pattern seen. + +```C +void func() { + grpc_error* error = GRPC_ERROR_CREATE("Some error occured"); + some_function(GRPC_ERROR_REF(error)); + // do things + some_other_function(GRPC_ERROR_REF(error)); + // do more things + some_last_function(error); +} +``` + +The last call takes ownership and will eventually give the error its final unref. + +When **implementing** a function that takes an error (and is not a `grpc_closure` callback function), you must ensure the error is unref-ed either by doing it explicitly with GRPC_ERROR_UNREF, or by passing the error to a function that takes over the ownership. From 257d4d68387b25d7ab31e9675e2634835d363c48 Mon Sep 17 00:00:00 2001 From: ncteisen Date: Fri, 17 Mar 2017 14:40:27 -0500 Subject: [PATCH 2/4] Regenerate project --- tools/doxygen/Doxyfile.core | 1 + tools/doxygen/Doxyfile.core.internal | 1 + 2 files changed, 2 insertions(+) diff --git a/tools/doxygen/Doxyfile.core b/tools/doxygen/Doxyfile.core index cd3f2af44c8..6c235cacf9b 100644 --- a/tools/doxygen/Doxyfile.core +++ b/tools/doxygen/Doxyfile.core @@ -770,6 +770,7 @@ doc/compression_cookbook.md \ doc/connection-backoff-interop-test-description.md \ doc/connection-backoff.md \ doc/connectivity-semantics-and-api.md \ +doc/core/error-ownership-rules.md \ doc/core/pending_api_cleanups.md \ doc/cpp-style-guide.md \ doc/environment_variables.md \ diff --git a/tools/doxygen/Doxyfile.core.internal b/tools/doxygen/Doxyfile.core.internal index e4f56f23747..ee34ec8a4b3 100644 --- a/tools/doxygen/Doxyfile.core.internal +++ b/tools/doxygen/Doxyfile.core.internal @@ -770,6 +770,7 @@ doc/compression_cookbook.md \ doc/connection-backoff-interop-test-description.md \ doc/connection-backoff.md \ doc/connectivity-semantics-and-api.md \ +doc/core/error-ownership-rules.md \ doc/core/pending_api_cleanups.md \ doc/cpp-style-guide.md \ doc/environment_variables.md \ From 031cfeb954f94d435ade4c4021a28a34987d789d Mon Sep 17 00:00:00 2001 From: ncteisen Date: Tue, 21 Mar 2017 19:07:17 -0700 Subject: [PATCH 3/4] Line breaks and doc reffing --- ...error-ownership-rules.md => grpc-error.md} | 89 ++++++++++++++----- src/core/lib/iomgr/error.h | 25 +----- 2 files changed, 69 insertions(+), 45 deletions(-) rename doc/core/{error-ownership-rules.md => grpc-error.md} (52%) diff --git a/doc/core/error-ownership-rules.md b/doc/core/grpc-error.md similarity index 52% rename from doc/core/error-ownership-rules.md rename to doc/core/grpc-error.md index a8ae072ae02..1fa24802315 100644 --- a/doc/core/error-ownership-rules.md +++ b/doc/core/grpc-error.md @@ -1,43 +1,72 @@ -gRPC Error Ownership Rules -===================== +# gRPC Error ## Background -`grpc_error` is the c-core's opaque representation of an error. It holds a collection of integers, strings, timestamps, and child errors that related to the final error. +`grpc_error` is the c-core's opaque representation of an error. It holds a +collection of integers, strings, timestamps, and child errors that related to +the final error. -grpc_errors are refcounted objects, which means they need strict ownership semantics. An extra ref on an error can cause a memory leak, and a missing ref can cause a crash. +always present are: -This document serves as a detailed overview of grpc_error's ownership rules. It should help people use the errors, as well as help people debug refcount related errors. +* GRPC_ERROR_STR_FILE and GRPC_ERROR_INT_FILE_LINE - the source location where + the error was generated +* GRPC_ERROR_STR_DESCRIPTION - a human readable description of the error +* GRPC_ERROR_TIME_CREATED - a timestamp indicating when the error happened + +An error can also have children; these are other errors that are believed to +have contributed to this one. By accumulating children, we can begin to root +cause high level failures from low level failures, without having to derive +execution paths from log lines. + +grpc_errors are refcounted objects, which means they need strict ownership +semantics. An extra ref on an error can cause a memory leak, and a missing ref +can cause a crash. + +This document serves as a detailed overview of grpc_error's ownership rules. It +should help people use the errors, as well as help people debug refcount related +errors. ## Clarification of Ownership -If a particular function is said to "own" an error, that means it has the responsibility of calling unref on the error. A function may have access to an error without ownership of it. +If a particular function is said to "own" an error, that means it has the +responsibility of calling unref on the error. A function may have access to an +error without ownership of it. -This means the function may use the error, but must not call unref on it, since that will be done elsewhere in the code. A function that does not own an error may explicitly take ownership of it by manually calling GRPC_ERROR_REF. +This means the function may use the error, but must not call unref on it, since +that will be done elsewhere in the code. A function that does not own an error +may explicitly take ownership of it by manually calling GRPC_ERROR_REF. ## Ownership Rules There are three rules of error ownership, which we will go over in detail. -* If `grpc_error` is returned by a function, the caller owns a ref to that instance. -* If a `grpc_error` is passed to a `grpc_closure` callback function, then that function does not own a ref to the error. -* if a `grpc_error` is passed to *any other function*, then that function takes ownership of the error. + +* If `grpc_error` is returned by a function, the caller owns a ref to that + instance. +* If a `grpc_error` is passed to a `grpc_closure` callback function, then that + function does not own a ref to the error. +* if a `grpc_error` is passed to *any other function*, then that function + takes ownership of the error. ### Rule 1 -> If `grpc_error` is returned by a function, the caller owns a ref to that instance.* +> If `grpc_error` is returned by a function, the caller owns a ref to that +> instance.* -For example, in the following code block, error1 and error2 are owned by the current function. +For example, in the following code block, error1 and error2 are owned by the +current function. ```C grpc_error* error1 = GRPC_ERROR_CREATE("Some error occured"); grpc_error* error2 = some_operation_that_might_fail(...); ``` -The current function would have to explicitly call GRPC_ERROR_UNREF on the errors, or pass them along to a function that would take over the ownership. +The current function would have to explicitly call GRPC_ERROR_UNREF on the +errors, or pass them along to a function that would take over the ownership. ### Rule 2 -> If a `grpc_error` is passed to a `grpc_closure` callback function, then that function does not own a ref to the error. +> If a `grpc_error` is passed to a `grpc_closure` callback function, then that +> function does not own a ref to the error. A `grpc_closure` callback function is any function that has the signature: @@ -53,7 +82,9 @@ c->cb(exec_ctx, c->cb_arg, err); The caller is still responsible for unref-ing the error. -However, the above line is currently being phased out! It is safer to invoke callbacks with `grpc_closure_run` and `grpc_closure_sched`. These functions are not callbacks, so they will take ownership of the error passed to them. +However, the above line is currently being phased out! It is safer to invoke +callbacks with `grpc_closure_run` and `grpc_closure_sched`. These functions are +not callbacks, so they will take ownership of the error passed to them. ```C grpc_error* error = GRPC_ERROR_CREATE("Some error occured"); @@ -61,7 +92,8 @@ grpc_closure_run(exec_ctx, cb, error); // current function no longer has ownership of the error ``` -If you schedule or run a closure, but still need ownership of the error, then you must explicitly take a reference. +If you schedule or run a closure, but still need ownership of the error, then +you must explicitly take a reference. ```C grpc_error* error = GRPC_ERROR_CREATE("Some error occured"); @@ -70,7 +102,11 @@ grpc_closure_run(exec_ctx, cb, GRPC_ERROR_REF(error)); GRPC_ERROR_UNREF(error); ``` -Rule 2 is more important to keep in mind when **implementing** `grpc_closure` callback functions. You must keep in mind that you do not own the error, and must not unref it. More importantly, you cannot pass it to any function that would take ownership of the error, without explicitly taking ownership yourself. For example: +Rule 2 is more important to keep in mind when **implementing** `grpc_closure` +callback functions. You must keep in mind that you do not own the error, and +must not unref it. More importantly, you cannot pass it to any function that +would take ownership of the error, without explicitly taking ownership yourself. +For example: ```C void on_some_action(grpc_exec_ctx *exec_ctx, void *arg, grpc_error *error) { @@ -78,7 +114,7 @@ void on_some_action(grpc_exec_ctx *exec_ctx, void *arg, grpc_error *error) { // and the caller of this callback will also unref it. some_function(error); - // this callback function must take ownership, so it can give that + // this callback function must take ownership, so it can give that // ownership to the function it is calling. some_function(GRPC_ERROR_REF(error)); } @@ -86,7 +122,8 @@ void on_some_action(grpc_exec_ctx *exec_ctx, void *arg, grpc_error *error) { ### Rule 3 -> if a `grpc_error` is passed to *any other function*, then that function takes ownership of the error. +> if a `grpc_error` is passed to *any other function*, then that function takes +> ownership of the error. Take the following example: @@ -97,9 +134,11 @@ some_function(error); // can't use error anymore! might be gone. ``` -When some_function is called, it takes over the ownership of the error, and it will eventually unref it. So the caller can no longer safely use the error. +When some_function is called, it takes over the ownership of the error, and it +will eventually unref it. So the caller can no longer safely use the error. -If the caller needed to keep using the error (or passing it to other functions), if would have to take on a reference to it. This is a common pattern seen. +If the caller needed to keep using the error (or passing it to other functions), +if would have to take on a reference to it. This is a common pattern seen. ```C void func() { @@ -112,6 +151,10 @@ void func() { } ``` -The last call takes ownership and will eventually give the error its final unref. +The last call takes ownership and will eventually give the error its final +unref. -When **implementing** a function that takes an error (and is not a `grpc_closure` callback function), you must ensure the error is unref-ed either by doing it explicitly with GRPC_ERROR_UNREF, or by passing the error to a function that takes over the ownership. +When **implementing** a function that takes an error (and is not a +`grpc_closure` callback function), you must ensure the error is unref-ed either +by doing it explicitly with GRPC_ERROR_UNREF, or by passing the error to a +function that takes over the ownership. diff --git a/src/core/lib/iomgr/error.h b/src/core/lib/iomgr/error.h index eb953947ae9..0e7a7b0a1ef 100644 --- a/src/core/lib/iomgr/error.h +++ b/src/core/lib/iomgr/error.h @@ -45,28 +45,9 @@ extern "C" { #endif /// Opaque representation of an error. -/// Errors are refcounted objects that represent the result of an operation. -/// Ownership laws: -/// if a grpc_error is returned by a function, the caller owns a ref to that -/// instance -/// if a grpc_error is passed to a grpc_closure callback function (functions -/// with the signature: -/// void (*f)(grpc_exec_ctx *exec_ctx, void *arg, grpc_error *error)) -/// then those functions do not own a ref to error (but are free to manually -/// take a reference). -/// if a grpc_error is passed to *ANY OTHER FUNCTION* then that function takes -/// ownership of the error -/// Errors have: -/// a set of ints, strings, and timestamps that describe the error -/// always present are: -/// GRPC_ERROR_STR_FILE, GRPC_ERROR_INT_FILE_LINE - source location the error -/// was generated -/// GRPC_ERROR_STR_DESCRIPTION - a human readable description of the error -/// GRPC_ERROR_TIME_CREATED - a timestamp indicating when the error happened -/// an error can also have children; these are other errors that are believed -/// to have contributed to this one. By accumulating children, we can begin -/// to root cause high level failures from low level failures, without having -/// to derive execution paths from log lines +/// See https://github.com/grpc/grpc/blob/master/doc/core/grpc-error.md for a +/// full write up of this object. + typedef struct grpc_error grpc_error; typedef enum { From 4c2e57df9afb9565dfd8c01272b08f24878de01c Mon Sep 17 00:00:00 2001 From: ncteisen Date: Tue, 21 Mar 2017 19:08:53 -0700 Subject: [PATCH 4/4] Regen project --- tools/doxygen/Doxyfile.core | 2 +- tools/doxygen/Doxyfile.core.internal | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/tools/doxygen/Doxyfile.core b/tools/doxygen/Doxyfile.core index 6c235cacf9b..94d962b5fe0 100644 --- a/tools/doxygen/Doxyfile.core +++ b/tools/doxygen/Doxyfile.core @@ -770,7 +770,7 @@ doc/compression_cookbook.md \ doc/connection-backoff-interop-test-description.md \ doc/connection-backoff.md \ doc/connectivity-semantics-and-api.md \ -doc/core/error-ownership-rules.md \ +doc/core/grpc-error.md \ doc/core/pending_api_cleanups.md \ doc/cpp-style-guide.md \ doc/environment_variables.md \ diff --git a/tools/doxygen/Doxyfile.core.internal b/tools/doxygen/Doxyfile.core.internal index ee34ec8a4b3..a4617d2e85e 100644 --- a/tools/doxygen/Doxyfile.core.internal +++ b/tools/doxygen/Doxyfile.core.internal @@ -770,7 +770,7 @@ doc/compression_cookbook.md \ doc/connection-backoff-interop-test-description.md \ doc/connection-backoff.md \ doc/connectivity-semantics-and-api.md \ -doc/core/error-ownership-rules.md \ +doc/core/grpc-error.md \ doc/core/pending_api_cleanups.md \ doc/cpp-style-guide.md \ doc/environment_variables.md \