From d8adb45bae098fb128232717235f0ca2695a1aaf Mon Sep 17 00:00:00 2001 From: Lukasz Wojciechowski Date: Thu, 13 Sep 2018 17:20:48 +0200 Subject: [PATCH] Fix situations when request's job is not set It is a good practice to check if pointer is not nil, before dereferencing it. Verification if Job field of request is set in closeRequest is such situation. It is not an error situation if Worker state transition: RUN->MAINTENANCE or RUN->FAIL happen and a request's is not run by any Job. It can occur e.g. when worker is already booked for a Job (in RUN state) and creation of Job is not yet completed or failed. Change-Id: Ib1790fdaab293b9478dc67f86b69f04a6808c50b Signed-off-by: Lukasz Wojciechowski --- requests/requests.go | 14 ++++++++++++-- requests/requests_test.go | 24 ++++++++++++++++++++++-- requests/requests_workerchange_test.go | 6 ++---- 3 files changed, 36 insertions(+), 8 deletions(-) diff --git a/requests/requests.go b/requests/requests.go index 2e42061..065dbae 100644 --- a/requests/requests.go +++ b/requests/requests.go @@ -136,9 +136,13 @@ func (reqs *ReqsCollection) NewRequest(caps boruta.Capabilities, // all required conditions to close request are met. // The method must be called in reqs.mutex critical section. func (reqs *ReqsCollection) closeRequest(req *boruta.ReqInfo) { + req.State = boruta.DONE + if req.Job == nil { + // TODO log logic error, but continue service. + return + } worker := req.Job.WorkerUUID reqs.jobs.Finish(worker) - req.State = boruta.DONE } // CloseRequest is part of implementation of Requests interface. @@ -459,7 +463,13 @@ func (reqs *ReqsCollection) OnWorkerFail(worker boruta.WorkerUUID) { job, err := reqs.jobs.Get(worker) if err != nil { - panic("no job related to running worker") + // Nothing to be done on requests or jobs level, when worker had no job assigned. + // It is not an error situation if Worker state transition: + // RUN->MAINTENANCE or RUN->FAIL happens and a request is not run + // by any Job. It can occur e.g. when worker is already booked for + // a Job (in RUN state) and creation of Job is not yet completed + // or failed. + return } reqID := job.Req diff --git a/requests/requests_test.go b/requests/requests_test.go index 3405a04..d3d4ec1 100644 --- a/requests/requests_test.go +++ b/requests/requests_test.go @@ -207,12 +207,32 @@ func TestCloseRequest(t *testing.T) { assert.Equal(boruta.ReqState(boruta.DONE), rqueue.requests[reqid].State) rqueue.mutex.RUnlock() + // Add another another valid request. + reqid, err = rqueue.NewRequest(req.Caps, req.Priority, req.Owner, req.ValidAfter, req.Deadline) + assert.Nil(err) + assert.EqualValues(3, reqid) + // Simulate situation where request is in PROGRESS state, but no job for it exists. + reqinfo, err = rqueue.GetRequestInfo(reqid) + assert.Nil(err) + rqueue.mutex.Lock() + rqueue.requests[reqid].State = boruta.INPROGRESS + rqueue.requests[reqid].Job = nil + rqueue.queue.removeRequest(&reqinfo) + rqueue.mutex.Unlock() + // Close request. + err = rqueue.CloseRequest(reqid) + assert.Nil(err) + rqueue.mutex.RLock() + assert.EqualValues(3, len(rqueue.requests)) + assert.Equal(boruta.ReqState(boruta.DONE), rqueue.requests[reqid].State) + rqueue.mutex.RUnlock() + // Simulation for the rest of states. states := [...]boruta.ReqState{boruta.INVALID, boruta.CANCEL, boruta.TIMEOUT, boruta.DONE, boruta.FAILED} reqid, err = rqueue.NewRequest(req.Caps, req.Priority, req.Owner, req.ValidAfter, req.Deadline) assert.Nil(err) - assert.EqualValues(3, reqid) + assert.EqualValues(4, reqid) reqinfo, err = rqueue.GetRequestInfo(reqid) assert.Nil(err) rqueue.mutex.Lock() @@ -228,7 +248,7 @@ func TestCloseRequest(t *testing.T) { rqueue.mutex.RLock() defer rqueue.mutex.RUnlock() - assert.EqualValues(3, len(rqueue.requests)) + assert.EqualValues(4, len(rqueue.requests)) assert.EqualValues(0, rqueue.queue.length) } diff --git a/requests/requests_workerchange_test.go b/requests/requests_workerchange_test.go index 01b5024..06b438c 100644 --- a/requests/requests_workerchange_test.go +++ b/requests/requests_workerchange_test.go @@ -93,11 +93,9 @@ var _ = Describe("Requests as WorkerChange", func() { }) }) Describe("OnWorkerFail", func() { - It("should panic if jobs.Get fails", func() { + It("should return if jobs.Get fails", func() { jm.EXPECT().Get(testWorker).Return(nil, testErr) - Expect(func() { - R.OnWorkerFail(testWorker) - }).To(Panic()) + R.OnWorkerFail(testWorker) }) It("should panic if failing worker was processing unknown Job", func() { noReq := boruta.ReqID(0) -- 2.7.4