In looking through the sssd code, there are instances where an asynchronous operation requires multiple tasks to be done in series (not to be confused with a parent request with multiple child requests). An example of a series of such tasks would look like: * send() * task1() * task2() * done()
While one would expect that the tasks would be named so as to describe their behavior, they are typically named as completing a previous task, giving no indication what the task is going to do. This can be confusing when reading the logs. For example, if the problem is that kinit failed, an entry in the log might have the function name "sdap_cli_rootdse_done", which doesn't help describe the kinit problem (the call stack below shows how "sdap_cli_rootdse_done" calls "sdap_cli_kinit_step").
In the following call stack, I have used the "bad name" annotation for those function names that don't describe the function behavior. I have also suggested using a more descriptive name that describes the functions behavior (each using a <suffix>). I'm not sure what the <suffix> should be; maybe "task" or "phase" or something.
sdap_cli_connect_send sdap_cli_resolve_next sdap_cli_resolve_done bad name; sdap_cli_connect_<suffix> would be better (maybe sdap_cli_connect_task) sdap_connect_send sdap_cli_connect_done bad_name; sdap_cli_rootdse_<suffix> would be better sdap_cli_rootdse_step sdap_cli_rootdse_done bad_name; sdap_cli_kinit_<suffix> would be better sdap_cli_kinit_step sdap_cli_kinit_done bad_name; sdap_cli_auth_<suffix> would be better sdap_cli_auth_step sdap_cli_auth_done bad_name; sdap_cli_connect_done would be better tevent_req_done
In addition to being non-descriptive, the "done" suffix is currently used in two different ways, which can be confusing: * as shown above, "done" is used for a series of intermediate tasks, whose names are based on previous tasks * in a simpler scenario (with no intermediate tasks), "done" is used to signify the final task (which makes sense); for example:
sdap_id_op_connect_send sdap_id_op_connect_step sdap_id_op_connect_done
To resolve both issues, I propose we improve our naming conventions by: * coming to consensus on a suffix for a series of intermediate tasks * declaring "done" to be used only for a final task (whether in a basic scenario, or a scenario with multiple intermediate tasks)
What do others think?
Regards, Yassir.
On Wed, Jan 15, 2014 at 05:20:58PM -0500, Yassir Elley wrote:
In looking through the sssd code, there are instances where an asynchronous operation requires multiple tasks to be done in series (not to be confused with a parent request with multiple child requests). An example of a series of such tasks would look like:
- send()
- task1()
- task2()
- done()
One word of warning about the tevent_req style. Some old code in the SSSD predates this style and both the functions and the accompanying structure can be named differently (the structure typically wouldn't be a *_state but rather a *_ctx).
While one would expect that the tasks would be named so as to describe their behavior, they are typically named as completing a previous task, giving no indication what the task is going to do. This can be confusing when reading the logs. For example, if the problem is that kinit failed, an entry in the log might have the function name "sdap_cli_rootdse_done", which doesn't help describe the kinit problem (the call stack below shows how "sdap_cli_rootdse_done" calls "sdap_cli_kinit_step").
In the following call stack, I have used the "bad name" annotation for those function names that don't describe the function behavior. I have also suggested using a more descriptive name that describes the functions behavior (each using a <suffix>). I'm not sure what the <suffix> should be; maybe "task" or "phase" or something.
sdap_cli_connect_send sdap_cli_resolve_next sdap_cli_resolve_done bad name; sdap_cli_connect_<suffix> would be better (maybe sdap_cli_connect_task) sdap_connect_send sdap_cli_connect_done bad_name; sdap_cli_rootdse_<suffix> would be better sdap_cli_rootdse_step sdap_cli_rootdse_done bad_name; sdap_cli_kinit_<suffix> would be better sdap_cli_kinit_step sdap_cli_kinit_done bad_name; sdap_cli_auth_<suffix> would be better sdap_cli_auth_step sdap_cli_auth_done bad_name; sdap_cli_connect_done would be better tevent_req_done
In addition to being non-descriptive, the "done" suffix is currently used in two different ways, which can be confusing:
- as shown above, "done" is used for a series of intermediate tasks, whose names are based on previous tasks
- in a simpler scenario (with no intermediate tasks), "done" is used to signify the final task (which makes sense); for example:
sdap_id_op_connect_send sdap_id_op_connect_step sdap_id_op_connect_done
To resolve both issues, I propose we improve our naming conventions by:
- coming to consensus on a suffix for a series of intermediate tasks
I'm not sure we can force a common suffix everywhere. I think this should be one of 'use your best judgement' cases. Maybe it would make sense to use past tense when the function ends with a verb (ie sdap_cli_connected) ?
- declaring "done" to be used only for a final task (whether in a basic scenario, or a scenario with multiple intermediate tasks)
What do others think?
I think the most important part of the tevent_req style is to have a common prefix and use it for all of _send/_done/_recv:
struct $prefix_state { };
$prefix_send(); $prefix_done(); $prefix_recv();
The intermediate 'steps' I'm not sure about, I don't think that needs to be formally codified.
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1
On 01/16/2014 04:25 PM, Jakub Hrozek wrote:
On Wed, Jan 15, 2014 at 05:20:58PM -0500, Yassir Elley wrote:
In looking through the sssd code, there are instances where an asynchronous operation requires multiple tasks to be done in series (not to be confused with a parent request with multiple child requests). An example of a series of such tasks would look like: * send() * task1() * task2() * done()
One word of warning about the tevent_req style. Some old code in the SSSD predates this style and both the functions and the accompanying structure can be named differently (the structure typically wouldn't be a *_state but rather a *_ctx).
While one would expect that the tasks would be named so as to describe their behavior, they are typically named as completing a previous task, giving no indication what the task is going to do. This can be confusing when reading the logs. For example, if the problem is that kinit failed, an entry in the log might have the function name "sdap_cli_rootdse_done", which doesn't help describe the kinit problem (the call stack below shows how "sdap_cli_rootdse_done" calls "sdap_cli_kinit_step").
In the following call stack, I have used the "bad name" annotation for those function names that don't describe the function behavior. I have also suggested using a more descriptive name that describes the functions behavior (each using a <suffix>). I'm not sure what the <suffix> should be; maybe "task" or "phase" or something.
sdap_cli_connect_send sdap_cli_resolve_next sdap_cli_resolve_done bad name; sdap_cli_connect_<suffix> would be better (maybe sdap_cli_connect_task) sdap_connect_send sdap_cli_connect_done bad_name; sdap_cli_rootdse_<suffix> would be better sdap_cli_rootdse_step sdap_cli_rootdse_done bad_name; sdap_cli_kinit_<suffix> would be better sdap_cli_kinit_step sdap_cli_kinit_done bad_name; sdap_cli_auth_<suffix> would be better sdap_cli_auth_step sdap_cli_auth_done bad_name; sdap_cli_connect_done would be better tevent_req_done
In addition to being non-descriptive, the "done" suffix is currently used in two different ways, which can be confusing: * as shown above, "done" is used for a series of intermediate tasks, whose names are based on previous tasks * in a simpler scenario (with no intermediate tasks), "done" is used to signify the final task (which makes sense); for example:
sdap_id_op_connect_send sdap_id_op_connect_step sdap_id_op_connect_done
To resolve both issues, I propose we improve our naming conventions by: * coming to consensus on a suffix for a series of intermediate tasks
I'm not sure we can force a common suffix everywhere. I think this should be one of 'use your best judgement' cases. Maybe it would make sense to use past tense when the function ends with a verb (ie sdap_cli_connected) ?
- declaring "done" to be used only for a final task (whether in a
basic scenario, or a scenario with multiple intermediate tasks)
What do others think?
I think the most important part of the tevent_req style is to have a common prefix and use it for all of _send/_done/_recv:
struct $prefix_state { };
$prefix_send(); $prefix_done(); $prefix_recv();
The intermediate 'steps' I'm not sure about, I don't think that needs to be formally codified.
This came out of a conversation that Yassir and I had the other day. One of the pieces that I see as being problematic in our style is just as he describes it: we're not following best-practice naming for our functions. The best practice is that each intermediate step in the execution should be named after what it is doing, not what it just finished doing. It makes no sense when looking at the debug logs to get an error reported in sdap_cli_rootdse_done() when the real action taking place is that we're attempting to perform a GSSAPI bind.
I agree with Yassir that I'd like to see a behavior more like the above, where we would have:
$prefix_send() $prefix_$description1_task() $prefix_$description2_task() ... $prefix_done()
And then as before, retrieve results with $prefix_recv()
This pattern would allow us to be more consistent, while also seeing trivially in the logs what operation we're performing and how far along we are in it. Yes, this will likely result in very long function names. I'm willing to accept that (our function names aren't exactly short right now anyway).
So to take a real-world example that I would be willing to refactor:
Here's the current operation (loops eliminated for brevity):
sdap_access_filter_send(): Establishes an sdap_id_op and connects to LDAP
sdap_access_filter_connect_done() Sends an LDAP base-search request for the user with a specialized filter to determine if the user will match it.
sdap_access_filter_get_access_done() Checks whether we received zero (denied) or one (granted) as a reply.
So in the proposed new pattern, this would become:
sdap_access_filter_send() sdap_access_filter_perform_filter_check_task() sdap_access_filter_done()
There's some question in my mind as well as to whether we might want the _done() function to also be more descriptive. In the above case, we might instead call it sdap_access_filter_authorization_done() (or *_authz_done())
sssd-devel@lists.fedorahosted.org