attachment
0001-Ticket-50-Add-ns_job_set_data-function.patch

Why is this needed? How is it intended to be used? Note that code comments in general should not ask questions - the comments (and the code itself) should answer them:
{{{
* Warning: This could be linked to memory leaks, the issue will be in the
* previous caller!!!
* Can we do something to help prevent these?
}}}
So the general pattern for using ns_job_set_data will be something like this:
{{{
void thing = ns_job_get_data(job);
free(thing); / but how do you know how to free it? */
ns_job_set_data(newthing);
}}}
Rather than have this sort of code duplication all over the place, and if indeed ns_job_get_data() is necessary for some reason, I think that every place where a job can be created with callback data, such as ns_add_io_job, ns_add_timeout_job, ns_add_io_timeout_job, ns_add_signal_job, and ns_add_job, should add a destructor function that knows how to free data correctly, including locking if necessary. Then the application can just call ns_job_set_data(newthing) and the old thing, if set, will be correctly freed first. You could also use the destructor in internal_ns_job_done, or even make it optional to use it there, as is done for the FD.

Replying to [comment:2 rmeggins]:

Why is this needed? How is it intended to be used? Note that code comments in general should not ask questions - the comments (and the code itself) should answer them:

Are there not places in NS where the absence of the comment does raise a question also? There are lots of edge cases and issues to consider.

Consider someone tracking their leak, and looking in GDB. This comment will now answer, that they must look elsewhere for the origin!

As well, questions are a useful way for us as a team in the future to ponder a better solution, rather than remaining ignorant of the pit falls. Indeed it prompted you to comment on a way we can handle this better, so I think the comment did it's job!

{{{
* Warning: This could be linked to memory leaks, the issue will be in the
* previous caller!!!
* Can we do something to help prevent these?
}}}
So the general pattern for using ns_job_set_data will be something like this:
{{{
void thing = ns_job_get_data(job);
free(thing); / but how do you know how to free it? */
ns_job_set_data(newthing);
}}}

Yes.

Rather than have this sort of code duplication all over the place, and if indeed ns_job_get_data() is necessary for some reason, I think that every place where a job can be created with callback data, such as ns_add_io_job, ns_add_timeout_job, ns_add_io_timeout_job, ns_add_signal_job, and ns_add_job, should add a destructor function that knows how to free data correctly, including locking if necessary. Then the application can just call ns_job_set_data(newthing) and the old thing, if set, will be correctly freed first. You could also use the destructor in internal_ns_job_done, or even make it optional to use it there, as is done for the FD.

I was going to raise the destructor ticket also, as I need this to prevent a memory leak in my test application.

Perhaps after I have added the destructor, I will re-visit this patch.

Very quickly, at second though, the idea of the set_data calling the destructor, doesn't that violate:

{{{
This data is private to the application - nunc-stans does not touch it in any way.
The application is responsible for managing the lifecycle of this data.
}}}

So either we need to change our comment on this, or we need to leave it up to the application.

Either way, we will need the destructor, because without it, we don't have a way to free job->data during shutdown of the threadpool.

Replying to [comment:4 firstyear]:

Very quickly, at second though, the idea of the set_data calling the destructor, doesn't that violate:

{{{
This data is private to the application - nunc-stans does not touch it in any way.
The application is responsible for managing the lifecycle of this data.
}}}

So either we need to change our comment on this, or we need to leave it up to the application.

No. Note that nunc-stans itself doesn't touch the data, the dtor does. This isn't any different than nunc-stans passing the application private data to the job callback function. The dtor is just another job callback function, although with a specific purpose.

Either way, we will need the destructor, because without it, we don't have a way to free job->data during shutdown of the threadpool.

ns_job_set_data, with or without a dtor, concerns me. But, as you say, I don't know of another way to free job->data at shutdown.

Updated patch with tests
0001-Ticket-50-Add-ns_job_set_data-helper.patch

I have done this still without the destructor, because I think that if you get / and replace, the CALLER should free the data.

I think that the ns_job_done callback will be the right place for the caller to place a destructor cb. This way we don't need to worry so much about it.

For the comment/doc string for ns_job_set_data(), you should add a code block that shows an example of ns_job_get_data, free, and ns_job_set_data with the new data object, and say that this idiom should always be followed, unless you are quite sure that ns_job_get_data returns NULL, or has some sort of data that does not need to be freed.

{{{
/ Is there a way to block til this job is done? /
sleep(1);
}}}
Yes - you will need to use a mutex+condition variable. Instead of the sleep you will wait on the condition variable, and ns_init_test_job_cb will notify the condition variable.

okay, I'll update this patch with those.

Update 3 based on Rich's new comments
0001-Ticket-50-Add-ns_job_set_data-helper.2.patch

Update 4, fix condvar delay issue.
0001-Ticket-50-Add-ns_job_set_data-helper.3.patch

{{{

659  *      if (data != NULL) { 
660  *          ... 
661  *          free(data); 
662  *      }

}}}
You don't need to test for NULL here - man 3 free:
{{{
The free() function frees the memory space pointed to by ptr, which
must have been returned by a previous call to malloc(), calloc(), or
realloc(). Otherwise, or if free(ptr) has already been called before,
undefined behavior occurs. If ptr is NULL, no operation is performed.
}}}
i.e. free(NULL) is a no-op.

{{{

111     /* This interval is in 1000ths of seconds */ 
112     PR_WaitCondVar(cb_cond, 1000);

}}}
The internal is not necessarily in milliseconds. Unless you are going to use one of the "special" values for interval PR_INTERVAL_NO_TIMEOUT or PR_INTERVAL_NO_WAIT you should convert a "real" time unit to an interval using one of the PR_ToInterval like PR_SecondsToInterval, PR_MillisecondsToInterval, PR_MicrosecondsToInterval:
{{{
PR_WaitCondVar(cb_cond, PR_SecondsToInterval(1)); / wait at most 1 second /
}}}

The dependency on cmocka - is that available on Fedora, RHEL, and CentOS?

Replying to [comment:10 rmeggins]:

{{{

659 * if (data != NULL) {
660 * ...
661 * free(data);
662 * }
}}}
You don't need to test for NULL here - man 3 free:
{{{
The free() function frees the memory space pointed to by ptr, which
must have been returned by a previous call to malloc(), calloc(), or
realloc(). Otherwise, or if free(ptr) has already been called before,
undefined behavior occurs. If ptr is NULL, no operation is performed.
}}}
i.e. free(NULL) is a no-op.

I'll fix that now.

{{{

111 / This interval is in 1000ths of seconds /
112 PR_WaitCondVar(cb_cond, 1000);
}}}
The internal is not necessarily in milliseconds. Unless you are going to use one of the "special" values for interval PR_INTERVAL_NO_TIMEOUT or PR_INTERVAL_NO_WAIT you should convert a "real" time unit to an interval using one of the PR_ToInterval like PR_SecondsToInterval, PR_MillisecondsToInterval, PR_MicrosecondsToInterval:

{{{
PR_WaitCondVar(cb_cond, PR_SecondsToInterval(1)); / wait at most 1 second /
}}}

Ahhh, I'll use that macro then.

However, I don't want to use the NO_TIMEOUT, NO_WAIT else the test won't return.

The dependency on cmocka - is that available on Fedora, RHEL, and CentOS?

Yes.

sudo yum install -y libcmocka libcmocka-devel. We can add them to build requires.

Update 5, edit doxygen notes, use pr seconds to interval.
0001-Ticket-50-Add-ns_job_set_data-helper.4.patch

the doxygen note is still doing if (data != NULL)

ok - I see what you're doing in the comment

in the test
{{{
126 strcpy(data, "first");
127
128 assert_int_equal(
129 ns_add_job(tp, NS_JOB_NONE|NS_JOB_THREAD, ns_init_test_job_cb, data, &job),
130 0);
131
132 / Check that the data is correct /
133 char retrieved = (char )ns_job_get_data(job);
134 assert_int_equal(strcmp("first", retrieved), 0);
}}}
Are you just checking to see if the job callback altered the data?
What about a specific job callback to test this function e.g. set data to "first", then have the job callback free it and set it to "second" and notify the condvar, then have ns_set_data_test wait for the condvar and verify that data is set to "second". Or something like that.

Yes, the test is to "prove" that the data is not tampered with.

Sure, we could expand to have a second test case that shows the work flow in the way you describe.

that test can be added later

commit 63e2cec3bcc3c0ccd4d1434e7405366af3cbfbc1
Writing objects: 100% (8/8), 2.97 KiB | 0 bytes/s, done.
Total 8 (delta 5), reused 0 (delta 0)
To ssh://git.fedorahosted.org/git/nunc-stans.git
1731d75..63e2cec master -> master