Add high level job object wrapper #877
Conversation
|
@kevpar @katiewasnothere @ambarve If anyone has time tomorrow, could you give this a glance |
dcantah
force-pushed
the
jobwrapper
branch
2 times, most recently
from
f8d9d8a to
c398a8a
Compare
| for { | ||
| err := winapi.GetQueuedCompletionStatus(iocpHandle, &code, &key, (**windows.Overlapped)(unsafe.Pointer(&overlapped)), windows.INFINITE) | ||
| if err != nil { | ||
| log.G(ctx).WithError(err).Error("failed to poll for job object message") |
There was a problem hiding this comment.
What happens if this is some non-transient error i.e some error which won't be resolved by the time of next iteration? We will keep looping with no way of breaking out of the loop?
There was a problem hiding this comment.
From asking around, I couldn't find such a case for this call so I didn't worry about it. In go-winio, we check if nothing was filled in for the overlapped struct and if not we just panic so I think this is a little more friendly at least.https://github.com/microsoft/go-winio/blob/5b44b70ab3ab4d291a7c1d28afe7b4afeced0ed4/file.go#L162
There was a problem hiding this comment.
That sounds fine. Do you think we should add a check for context Done()and break from this loop in that case?
There was a problem hiding this comment.
Nevermind, that's a bad idea. The ctx which was actually used during a call to this function could be cancelled but then that will stop this loop for all other jobs.
internal/queue/mq.go
Outdated
| mq.closed = true | ||
| // If there's anybody currently waiting on a value from ReadOrWait, we need to | ||
| // signal so the read can return ErrQueueClosed. | ||
| mq.c.Signal() |
There was a problem hiding this comment.
I am still kind of confused about this. What happens if multiple goroutines call ReadOrWait() and are waiting for a notification to arrive but someone closes the queue before that. With the Signal call we will only wake one of them from the sleep right? Shouldn't all the goroutines wake up from their sleep and return ErrQueueEmpty?
We don't have any tests that tests this scenario.
There was a problem hiding this comment.
So this should be broadcast, good call. For the write case it should just be signal but agreed here we should wake up everyone.
There was a problem hiding this comment.
Yeah, for the write case we should only wake up one thread with a Signal call. Maybe I typed this comment near the write function in previous version and that caused the confusion. Haha.
|
One small fix about using |
* Add high level job object wrapper. * Add extra job object bindings for stats usage. Signed-off-by: Daniel Canter <[email protected]>
|
@kevpar If you have some time to do a last sweep :) |
Related work items: microsoft#173, microsoft#839, microsoft#856, microsoft#877, microsoft#881, microsoft#886, microsoft#887, microsoft#888, microsoft#889, microsoft#890, microsoft#893, microsoft#894, microsoft#896, microsoft#899, microsoft#900, microsoft#902, microsoft#904, microsoft#905, microsoft#906, microsoft#907, microsoft#908, microsoft#910, microsoft#912, microsoft#913, microsoft#914, microsoft#916, microsoft#918, microsoft#923, microsoft#925, microsoft#926, microsoft#928, microsoft#929, microsoft#932, microsoft#933, microsoft#934, microsoft#938, microsoft#939, microsoft#942, microsoft#943, microsoft#945, microsoft#946, microsoft#947, microsoft#949, microsoft#951, microsoft#952, microsoft#954
Add high level job object wrapper
Signed-off-by: Daniel Canter [email protected]