refactor(job)!: eliminate data race; add callback

[reugn]

The PR discussion is here - (#77).

[codecov-commenter]

Codecov Report

Merging #76 (f932b89) into master (f90eaa3) will decrease coverage by 0.35%.
Report is 1 commits behind head on master.
The diff coverage is 89.28%.

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

@@            Coverage Diff             @@
##           master      #76      +/-   ##
==========================================
- Coverage   90.04%   89.70%   -0.35%     
==========================================
  Files          10       10              
  Lines         643      709      +66     
==========================================
+ Hits          579      636      +57     
- Misses         43       50       +7     
- Partials       21       23       +2     

Files	Coverage Δ
quartz/function_job.go	100.00% <100.00%> (ø)
quartz/job.go	89.62% <85.48%> (-7.60%)	⬇️

... and 1 file with indirect coverage changes

[reugn]

@tychoish, @rfyiamcool, @tliron, it would be great to hear your thoughts on this.

[rfyiamcool]

this is a good pr . 👍 👍 👍

[tychoish]

the response here is still mutable: the mutex only prevents it from being set or unset when the function returns... another actor could modify the repose itself (in theory.)

[rfyiamcool]

This http.reponse is special, for example, after we read the http body, we can no read it again.

I think we can ignore this problem. 😁

[reugn]

Yes, the getter method has been added to eliminate data race during concurrent read/write operations.

[tychoish]

I'd be inclined to use the atomic.Int64 type/wrapper which ends up being clearer/easier...

[reugn]

Agree. But it is available starting from go1.19.

[tychoish]

I guess I don't see where, exactly the race was being triggered from the change? Something accessing the body of the job? I often try and fix races by avoiding passing pointers when you don't need mutability, rather than adding mutexes (and I try and avoid RW mutexes except as a last resort; these days)...

There are a lot of orthogonal changes scattered around this PR, while I'm a bit confused by it, I'm sure it's probably fine.

Was there something in particular you wanted my feedback on?

[reugn]

@tychoish, thanks for looking into this.

1. Exactly, there may be a case where a job is run by an executor while the user is simultaneously reading the result fields. I agree that sync.RWMutex is an overhead in this case, and is better to replace it with the regular sync.Mutex. atomic.Value is not a good choice here since we need atomicity for all the result fields. Let me know if you have a better idea considering the go1.18 constraint.
2. Another thing is the callback I added (agree, it would be better to put it in a separate PR). Do you see value in it?
3. Could you elaborate on:

fix races by avoiding passing pointers when you don't need mutability

[tychoish]

Another thing is the callback I added (agree, it would be better to put it in a separate PR). Do you see value in it?

I mean, callbacks are cool, though I think given that we're letting people execute arbitrary code (e.g. everything could be a function job, people could write functions that include post-operation-behavior, etc.) so it might not be always useful for users (but it might be useful for us, internally? (e.g. rescheduling/re-queuing could all happen in a callback?) but I actually tend to think that callbacks lead to harder to read code.

Let me know if you have a better idea considering the go1.18 constraint.

I agree that we shouldn't use atomic.Value here, sync.Mutex is probably fine (really), though I think at this point the go1.18 limitation is a bit aggressive. Maybe fix the bug, do a release, then bump the go version and have the next release make some of these changes more reasonably?

fix races by avoiding passing pointers when you don't need mutability

Oh, I think there are two different ways to fix data races: use concurrency control (mutexes/etc.) to limit access, and "make copies of data and pass them to each actor in the system and deal with the fact that actors might have different views of the system" (and one way you can do that is avoid the use of pointers so that go's dispatcher just makes the copies (probably on the stack) for you.

[rfyiamcool]

Is this okay? 😁

func (cu *CurlJob) Response() *http.Response {
	cu.RLock()
	defer cu.RUnlock()
        cu.response

	resp, _ := httputil.DumpResponse(cu.response, true)
        return resp 
}

https://pkg.go.dev/net/http/httputil#DumpRequest

[reugn]

https://pkg.go.dev/net/http/httputil#DumpRequest

@rfyiamcool, thank you. This eliminates potential data races. I've implemented it as:

func (cu *CurlJob) DumpResponse(body bool) ([]byte, error) {
	cu.Lock()
	defer cu.Unlock()
	return httputil.DumpResponse(cu.response, body)
}