feat(libooniengine): add support for running tasks #1112

DecFox · 2023-02-22T06:48:45Z

Checklist

I have read the contribution guidelines
reference issue for this pull request: engine: add support for running tasks probe#2416
if you changed anything related to how experiments work and you need to reflect these changes in the ooni/spec repository, please link to the related ooni/spec pull request:
if you changed code inside an experiment, make sure you bump its version number

Description

This diff adds support for running tasks in libooniengine which are subject to interaction via the FFI consumer. (edited)

bassosimone

I think we should temporarily stash part of the code we have in this pull request and just focus on adding support for running tasks, without additional complexity that may distract us.

I think the right next-step is to just flesh out how the Dart consumer will interact with tasks.

bassosimone · 2023-03-17T10:17:19Z

internal/engine/session.go

@@ -221,6 +221,63 @@ func NewSession(ctx context.Context, config SessionConfig) (*Session, error) {
 	return sess, nil
 }

+func NewSessionWithoutTunnel(ctx context.Context, config *SessionConfig) (*Session, error) {


I am not convinced we need this new constructor

bassosimone · 2023-03-17T10:17:35Z

internal/engine/session.go

+		return nil, errors.New("SoftwareVersion is empty")
+	}
+	if config.KVStore == nil {
+		config.KVStore = &kvstore.Memory{}


You should not mutate a config passed by pointer.

bassosimone · 2023-03-17T10:20:16Z

internal/libooniengine/engine.h

+
+// OONITask


Suggested change

// OONITask

bassosimone · 2023-03-17T10:21:15Z

internal/libooniengine/engine.h

+///
+/// @return Zero on failure, nonzero on success. If the return value
+/// is nonzero, a task is running. In such a case, the caller is
+/// responsible to eventually dispose of the task using OONIEngineFreeMemory.


Are you sure we can use OONIEngineFreeMemory to dispose of a task? I thought we should be having a dedicated API that removes the corresponding handle from the corresponding map.

bassosimone · 2023-03-17T10:22:02Z

internal/libooniengine/engine.h

+///
+/// @return A NULL pointer on failure, non-NULL otherwise. If the return
+/// value is non-NULL, the caller takes ownership of the OONIMessage
+/// pointer and MUST free it using OONIMessageFree when done using it.


Shouldn't this say OONIEngineFreeMemory?

bassosimone · 2023-03-17T10:27:15Z

internal/libooniengine/logger.go

+	"github.com/ooni/probe-cli/v3/internal/model"
+)
+
+type LogLevel int32


Cannot these be strings? I think it's easier for a consumer if these are strings.

bassosimone · 2023-03-17T10:28:03Z

internal/libooniengine/logger.go

+func newTaskLogger(emitter taskMaybeEmitter) *taskLogger {
+	return &taskLogger{
+		emitter: emitter,
+		verbose: false,


The constructor should also initialize verbose.

bassosimone · 2023-03-17T10:28:33Z

internal/libooniengine/main.go

+	err := json.Unmarshal([]byte(C.GoString(req)), out)
+	if err != nil {


Suggested change

err := json.Unmarshal([]byte(C.GoString(req)), out)

if err != nil {

if err := json.Unmarshal([]byte(C.GoString(req)), out); err != nil {

bassosimone · 2023-03-17T10:30:50Z

internal/libooniengine/model.go

+	waitForNextEvent(timeout time.Duration) *response
+
+	// isDone implements OONITaskIsDone.
+	isDone() bool


Where are you implementing isDone?

bassosimone · 2023-03-17T10:32:56Z

internal/libooniengine/ticker.go

+
+// runTicker emits a ticker event every second. It is a subtask
+// associated with all tasks.
+func runTicker(ctx context.Context, close chan any,


Because we have a timeout associated with each WaitForNextEvent, we don't need a ticker.

DecFox · 2023-03-21T03:17:58Z

Thank you for the feedback! I have made the requested changes.

bassosimone

Did another pass and provided lots of suggestions!

Thanks for moving this forward! 🙌

bassosimone · 2023-03-28T13:14:45Z

internal/libooniengine/engine.h

+/// engine that performs a background operation and emits meaningful
+/// events such as, for example, the results of measurements.


This description should be updated to mention that we emit interim outputs such as logs and progress as well as the result of the operation.

bassosimone · 2023-03-28T13:15:37Z

internal/libooniengine/engine.h

-void OONIENgineFreeMemory(void *ptr);
+void OONIEngineFreeMemory(void *ptr);
+
+/// OONIEngineCall starts a new OONITask using the given [req].


When I wrote the prototype, I used [req] but it seems the right syntax when using Doxygen is @p req. Do you mind updating the documentation in this file?

bassosimone · 2023-03-28T13:28:12Z

internal/libooniengine/engine.h

+/// value is non-NULL, the caller takes ownership of the char pointer
+/// and MUST free it using OONIEngineFreeMemory when done using it.
+///
+/// This function will return an empty string:


I am confused by this documentation. It strikes me as bad to return NULL or an empty string or a valid string and it would be better for the return value to be either NULL or a valid JSON string. I think we should double check the implementation to be sure it actually does what is documented or whether the documentation was just inconsistent with the actual implementation. (TODO(@bassosimone))

bassosimone · 2023-03-28T13:29:06Z

internal/libooniengine/engine.h

+/// value is non-NULL, the caller takes ownership of the char pointer
+/// and MUST free it using OONIEngineFreeMemory when done using it.
+///
+/// This function will return an empty string:


I don't think we need a timeout for this function call. It should be called after the task is done, and there should be a result once this happens. So, we can simplify the life of the caller by removing support for the timeout.

bassosimone · 2023-03-28T13:32:37Z

internal/libooniengine/engine.h

+/// checking whether this function has returned an empty string.
+char *OONIEngineTaskGetResult(OONITask task, int32_t timeout);
+
+/// OONIEngineTaskIsDone returns whether the task identified by [taskID] is done. A taks is


Suggested change

/// OONIEngineTaskIsDone returns whether the task identified by [taskID] is done. A taks is

/// OONIEngineTaskIsDone returns whether the task identified by [taskID] is done. A task is

bassosimone · 2023-03-28T13:54:32Z

internal/motor/task.go

+	r := reflect.ValueOf(req)
+	t := r.Type()
+	for i := 0; i < r.NumField(); i++ {
+		if !r.Field(i).IsNil() {


IIUC, the Request contains structs and not pointers. Because of this, the check that would actually work here would be IsZero. In any case, I suggested pointers, so IsNil would be okay.

That said, I am not sure we should implement task dispatching using reflection like this.

bassosimone · 2023-03-28T13:55:52Z

internal/motor/task.go

+		result:  make(chan *Response, 1),
+		stopped: make(chan any),
+	}
+	go tp.main(ctx, name, req)


If we're dispatching by name, we should probably add a Name to the Request struct. If we want to dispatch by pointer, otherwise, we can avoid passing name field here and only check for the req fields.

bassosimone · 2023-03-28T13:58:15Z

internal/motor/task.go

+		return nil // timeout while blocking for read
+	case ev := <-tp.result:
+		return ev // block for read till we receive a result
+	}


as I mentioned when discussing the C API, this can just be return <-tp.result

bassosimone · 2023-03-28T13:59:51Z

internal/motor/task.go

+	for !tp.IsDone() {
+		const blockForever = -1
+		_ = tp.WaitForNextEvent(blockForever)
+	}


I am wondering if draining the channel is actually needed. IIUC, we always emit in nonblocking mode.

bassosimone · 2023-03-28T14:00:53Z

internal/motor/task.go

+	runner := taskRegistry[name]
+	if runner == nil {
+		log.Printf("OONITaskStart: unknown task name: %s", name)
+		return


We cannot just return here. We now always need to tp.result <- resp.

decfox added 2 commits February 22, 2023 12:15

feat: expose NewSession in the C API

ca3f41c

feat: introduce geolocate

7dd9f5d

bassosimone reviewed Mar 17, 2023

View reviewed changes

decfox added 2 commits March 20, 2023 09:31

refactor: dry out code to only include running tasks

468b313

changes from code review

95469ed

DecFox changed the title ~~feat(libooniengine): introduce the ability to bootstrap and close sessions~~ feat(libooniengine): add support for running tasks Mar 20, 2023

feat: add mock task for integration testing

542c4e3

decfox added 3 commits March 25, 2023 10:52

feat: separate Go and C code

719109a

feat: introduce TaskAPI.GetResult()

c73486f

refactor: remove redundant comments

000abe5

DecFox marked this pull request as ready for review March 25, 2023 06:56

DecFox requested a review from hellais as a code owner March 25, 2023 06:56

bassosimone reviewed Mar 28, 2023

View reviewed changes

decfox added 4 commits March 31, 2023 11:29

changes from code review

9b904d8

x

d0a225a

refactor: handle non-positive timeout explicitly

62ca51b

feat: introduce custom handle

e38a274

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(libooniengine): add support for running tasks #1112

feat(libooniengine): add support for running tasks #1112

DecFox commented Feb 22, 2023 •

edited

Loading

bassosimone left a comment

bassosimone Mar 17, 2023

bassosimone Mar 17, 2023

bassosimone Mar 17, 2023

bassosimone Mar 17, 2023

bassosimone Mar 17, 2023

bassosimone Mar 17, 2023

bassosimone Mar 17, 2023

bassosimone Mar 17, 2023

bassosimone Mar 17, 2023

bassosimone Mar 17, 2023

DecFox commented Mar 21, 2023

bassosimone left a comment

bassosimone Mar 28, 2023

bassosimone Mar 28, 2023

bassosimone Mar 28, 2023

bassosimone Mar 28, 2023

bassosimone Mar 28, 2023

bassosimone Mar 28, 2023

bassosimone Mar 28, 2023

bassosimone Mar 28, 2023

bassosimone Mar 28, 2023

bassosimone Mar 28, 2023

		err := json.Unmarshal([]byte(C.GoString(req)), out)
		if err != nil {

	err := json.Unmarshal([]byte(C.GoString(req)), out)
	if err != nil {
	if err := json.Unmarshal([]byte(C.GoString(req)), out); err != nil {

		/// engine that performs a background operation and emits meaningful
		/// events such as, for example, the results of measurements.

	/// OONIEngineTaskIsDone returns whether the task identified by [taskID] is done. A taks is
	/// OONIEngineTaskIsDone returns whether the task identified by [taskID] is done. A task is

feat(libooniengine): add support for running tasks #1112

Are you sure you want to change the base?

feat(libooniengine): add support for running tasks #1112

Conversation

DecFox commented Feb 22, 2023 • edited Loading

Checklist

Description

bassosimone left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DecFox commented Mar 21, 2023

bassosimone left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DecFox commented Feb 22, 2023 •

edited

Loading