WIP: feat: add the ability to cancel params

[dylanhitt]

Here is PR for #41

If thi is acceptable I'd polish it up some and at least get through all of the device functions. I needed this sooner rather than later.

[NSEcho]

Hey there @dylanhitt , I was a bit busy so couldn't really answer everything. I still it is better to use context instead of cancellable. I will send the example shortly.

[NSEcho]

Along these lines:

Implementation:

func (d *Device) ParamsWithContext(ctx context.Context) (map[string]any, error) {
	type paramsRet struct {
		dt  map[string]any
		err error
	}
	ch := make(chan *paramsRet)
	go func() {
		dt, err := d.Params()
		ch <- &paramsRet{
			dt:  dt,
			err: err,
		}
	}()

	for {
		select {
		case <-ctx.Done():
			go func() {
				<-ch
			}()
			return nil, ErrContextCancelled
		case ret := <-ch:
			return ret.dt, ret.err
		}
	}
}

Usage:

ctx, _ := context.WithTimeout(context.Background(), 1*time.Second) // or whatever time or other option to cancel
params, err := device.ParamsWithContext(ctx)

Additionally, we can create context.WithCancel, run the function in goroutine and cancel it on some our condition.

Let me know what do you think about this.

[NSEcho]

Also, this way we don't need to rewrite each existing function, just create context wrapper around it.

[dylanhitt]

Are you missing passing GCancellable to frida_device_query_system_parameters_sync? The style of the code above is how I came across the issue. In this case you're simply exiting the goroutine and cleaning up all the go objects not any of the frida resources. You can do the above but you will need to pass Gcancellable between the funcs. and pass it into the call of C.frida_device_query_system_parameters_sync. You will then need to reference that same GCancellable within your select{} like:

for {
		select {
		case <-ctx.Done():
			go func() {
				<-ch
			}()
                         C.g_cancellable_cancel(*C.GCancellable)
			return nil, ErrContextCancelled
		case ret := <-ch:
                         // will also need to unref GCancellable since it wasn't cancelled
			return ret.dt, ret.err
		}
	}

I agree with you that having a ParamsCtx style thing is nice and is much more idiomatic. Hiding this GCancellable flow away from users would be great. We definitely should do it as most people won't need this type of granularity with GCancellable.

Another note for why you'd want expose GCanceallable is so that users making multiple frida calls the user can cancel the whole operation at anytime using the same GCancel

[NSEcho]

Honestly I do not see good solution as I would like ease of use to be priority. I do not like so many channels in main function just to cancel some operation.

[dylanhitt]

I'm not sure I'm following? I don't think the solution I'm proposing uses any channels.

[NSEcho]

Referencing the last comment about main.go here #41 (comment)

[dylanhitt]

Oh. That's just the implementation I chose to show you an example. I mean there really is no other way to cancel a blocking function in go. At least using the frida synchronous functions. Switching to the async frida funcs would allow for that flexibility but that again would require more pressure on the caller. That being said you can still use the code im proposing just like:

d := frida.NewDeviceManager()
d.EnumerateDevices()

fridaDevice, err := d.FindDeviceByID("my-device")
if err != nil {
	log.Fatal(err)
}

params, err := fridaDevice.Params()
if err != nil {
  panic(err)
}

fmt.Println(params)

[dylanhitt]

With that above extending it slightly hide the details of GCancellable from a standard user is fairly strightforward. The ParamsCtx that you're proposing would turn into this:

func (d *Device) ParamsCtx(ctx context.Context) (map[string]any, error) {
	type paramsRet struct {
		dt  map[string]any
		err error
	}
	c := frida.NewCancellable()
	ch := make(chan *paramsRet)
	go func() {
		dt, err := d.Params(frida.WithCancel(c))
		ch <- &paramsRet{
			dt:  dt,
			err: err,
		}
	}()

	for {
		select {
		case <-ctx.Done():
			go func() {
				<-ch
			}()
                         c.Cancel() <---- you have to cancel here are you're leaking. 
			return nil, ErrContextCancelled
		case ret := <-ch:
                         C.Unref()
			return ret.dt, ret.err
		}
	}
}

[NSEcho]

Yeah, this looks okay, but how we could achieve both things, using plain context with hiding out the cancellable along with the option for the user to use a single cancellable for the entire thing like you mentioned in some comment previously. Btw, I really like the idea, just trying to figure out what would be the best for the user and Frida.

[dylanhitt]

So if the user wants to string many together using the same cancelable there is no way to prevent code that looks like this:

c := frida.NewCancellable()
	ch := make(chan *paramsRet)
	go func() {
		dt, err := d.Params(frida.WithCancel(c))
		dt, err := d.OtherFunc(frida.WithCancel(c))
                 dt, err := d.Another(frida.WithCancel(c))
                 // eventually retrurn something down the channel.
	}()

	for {
		select {
		case <-ctx.Done():
			go func() {
				<-ch
			}()
                         c.Cancel() <---- you have to cancel here or you're leaking. 
			return nil, ErrContextCancelled
		case ret := <-ch:
                         C.Unref()
			return ret.dt, ret.err
		}
	}

The caller will have to handle this type of stuff themselves. That's just the nature of this. Many users will probably never write code like this or really even need too. But for instance with current state of ios 17 being able to cancel specific device functions is vital. For the majority of users chaining context functions without any channels would be all they would need.

[dylanhitt]

We could provide some trickery with some function they could pass into wrap all the commands like:

c := frida.NewCancellable()
runMultipleFuncWithCtx(ctx context.Context, func() {
                 dt, err := d.Params(frida.WithCancel(c))
		dt, err := d.OtherFunc(frida.WithCancel(c))
                 dt, err := d.Another(frida.WithCancel(c))
})

And we can handle the ctx stuff for them, but with this. You will need to still maintain the api I'm proposing.

[NSEcho]

Alright then, feel free to implement this. It is much more clear now.

[dylanhitt]

Alright, thank you for staying with me while I explained it. I'm not the best at getting my thoughts out. I went ahead and pushed up a commit showing what this would look like with ParamsCtx as well. I think that would be best as a separate PR? This is just to show you want this would like in the end. I'm wondering if we can do something tricky here Ctx functions as their is quite a but of boilerplate there.

[NSEcho]

I guess we can wrap it in a single function and use that instead of writing the same code all over again. If you are okay with this, I would like to merge it.

[dylanhitt]

Sure, I'll probably implement a few more over the next few days. Thanks