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 <l.wojciechow@partner.samsung.com>

diff --git a/requests/requests.go b/requests/requests.go
index 2e42061..065dbae 100644 (file)
--- 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 (file)
--- 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 (file)
--- 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)