Re: How to design HTTP request between HTTP servers

[Brian Candler]

> Any help and comments about my application design

A few general observations first:

1. In several places you are ignoring the error status, in particular at json.Unmarshal(bodybytes, &nfp) and the error from transport.Client.Do

2. In HandlePUTMessage you are ignoring the HTTP status code

3. I can't see why you're using transport.Client.Do in one function, and Client.Do in another

4. The "Location" response header, that you read in SendPUTMessageToServerB, should only be set on 3xx (redirect) responses,  but you only accept 200 / 204

5. Your main function starts a goroutine ("go SendPATCHMessageEveryTimerValue()"), but then immediately exits: https://play.golang.org/p/dLXvxfCaki4 .  If there's no other stuff going on, then just remove the "go".  (But maybe this is because this is an incomplete program fragment, and your real main() does other stuff)

6. You have clearly realised that you can't have two different goroutines reading and writing the same variable concurrently.  However the way you've used mutex seems wrong here. Firstly, you're keeping the mutex locked indefinitely (you should only lock it for a short time, while performing an action that touches those variables), and secondly, I don't see any code in the main goroutine which would be touching the same data concurrently - which would also need to be protected by the mutex. But again, this could also be because the program is incomplete.

> restarting from step 1 when 404 status code is received does not work

I believe the problem is that you're not returning an "error" status from PATCHMessage under that condition.  You can synthesise one of your own:

   } else if status == http.StatusNotFound {

        // Here I would like to restart PUT message if status code is 404
       return fmt.Errorf("Got http.StatusNotFound")
    }

... which would then be caught by "if err != nil" in SendPATCHMessageEveryTimerValue.

If you want, you could make your own object (compatible with the "error" interface) to carry the HTTP status as a value:

type HttpError struct {

        Code        int

        Description string

}

func (e HttpError) Error() string {

        return e.Description

}

...

        if status != http.StatusOK {

                return &HttpError{

                        Code:        resp.StatusCode,

                        Description: fmt.Sprintf("Unexpected status: %s", resp.Status),

                }

    }    

This can be useful if the caller wants to see the HTTP status value and act on it:

        err := Foo()

        if e, ok := err.(*HttpError); ok && e.Code == http.StatusForbidden {
                .... do something
        }

However, personally, I think you should structure the code more closely along the lines of your original description of the problem.

What I mean is: instead of having SendPATCHMessageEveryTimerValue internally call out to SendPUTMessage if there's an error, I'd make it terminate.  The caller is then responsible for going back to step 1 and sending the PUT before sending more PATCHes.

This gives the overall structure something like this:

func PutAndPatch() {

    for {

        var interval time.Duration

        for {
            ... do your step 1, 2, 3

            ... break out when you have the valid PUT response and have set 'interval'

        }

        for {

            ... do your step 4, 5, 6

            ... break out when there is a failure to PATCH

        }

    }

}

The caller can then choose to run this as a go routine, with "go PutAndPatch()", or not, as they prefer.  Note also that the state ("var interval") is private to this function, so there's no need for a mutex.  This would also allow you to run multiple instances of PutAndPatch, talking to different servers, in different goroutines.

Of course, as it stands, if you run PutAndPatch() in a goroutine then it will run autonomously, and the main program will have no idea of its status - and not even have a way to stop it.  If you want to add that sort of functionality, then the cleanest way is likely to be using channels.   For example, the standard way to signal a "stop" is to have an empty channel, which the caller closes when they want the stop to take place; or to use the "context" library which is a wrapper around this idea.  For any sort of concurrency it's also well worth getting your head around the contents of this video: GopherCon 2018: Bryan C. Mills - Rethinking Classical Concurrency Patterns.

If you do decide to read and/or modify global state from PutAndPatch(), then you'd want to lock the mutex just before accessing it and unlock it immediately afterwards - and the main program would need to do the same.  It's very easy to introduce bugs if you forget to do this.  Use the race detector to help find such problems.

HTH,

Brian.

[Brian Candler]

One other minor point.  When you write

        for _ = range ticker.C {
            ...
        }

then you have a timer leak if you break or return from that loop but don't call ticker.Stop().  If you only ever exit by "return" from the function, then you can simply do:

        ticker := time.NewTicker(time.Duration(Timer) * time.Second)
        defer ticker.Stop()

as per the example here.  However since your code could never exit from that loop, it wasn't a problem (and the outer "for { ... }" loop was unnecessary too)

[Brian Candler]

On Saturday, 20 March 2021 at 10:04:33 UTC Brian Candler wrote:

2. In HandlePUTMessage you are ignoring the HTTP status code

My mistake, ignore that one.  You are handling it in the caller, SendPUTMessageToServerB.

[Brian Candler]

On Saturday, 20 March 2021 at 19:19:53 UTC Van Fury wrote:

OK, if I understand you correctly, the outer for loop is not needed and that

there should be two for loops.

the first for loop does step 1,2 and 3 and then "break" out of the loop when valid response is received

and then set the "interval".

The second for loop start a for _ := range ticker.C loop and also break out when there is a PATCH failure response.

And then you'll need an outer for loop around that, so that the PATCH failure response causes you to restart at step 1.

[Brian Candler]

On Sunday, 21 March 2021 at 11:20:40 UTC Van Fury wrote:

After this , the PATCH message is send OK but when server B stop running,

server A break, complain g about the status check in the PATCH for loop

status204ERRO[0350] Patch "http://127.0.0.1:9090/nnrf-nfm/v1/nf-instances/6ba7b810-9dad-11d1-80b4-00c04fd430c9": dial tcp 127.0.0.1:9090: connect: connection refused
panic: runtime error: invalid memory address or nil pointer dereference
[signal SIGSEGV: segmentation violation code=0x1 addr=0x0 pc=0xe6aa92]

goroutine 47 [running]:
PutAndPatch()
        register.go:147 +0x1d2
exit status 2

Can you show the code at line 147 of register.go, with two or three lines of context either side?

My guess is that your problem is here:

        interval, cl, resp = SendNFInstanceRegistration()

        status := resp.StatusCode

or here:

   htbt, contentLoc, resp, err := HandleNFInstanceRegistration(locProfilebytes, locVarUrl)
    if err != nil {
        logrus.Error("Server A could not register profile")
    }

    status := resp.StatusCode

In either case, what happens if resp is nil?  You have checked for the case of err != nil, but you only make a log message - and then continue blithely on as if everything was successful.  But in the case of error, resp is likely to be nil, and you can't dereference a nil pointer.  That's what causes the panic.

Also: somebody has to be responsible for closing the response.  You're not explicitly returning an error from SendNFInstanceRegistration() to HandleNFInstanceRegistration(), so the only way you can indicate that the body is invalid is to return nil as the response pointer.

        interval, cl, resp = SendNFInstanceRegistration()
        if resp == nil {

            ... log if you like

            return ...

        }
        defer resp.Body.Close()
        status := resp.StatusCode

        .... continue to parse the response

 

Personally I think it would be cleaner to return an explicit "err", as it's consistent with the normal conventions.

You still aren't checking the error response from json.Unmarshal(bodybytes, &nfprofile).  If the JSON parsing fails then you'll get a zero response for the timer interval, and a ticker with a zero interval will cause a panic.

I think I'll leave it with you now.  In short: check errors everywhere; take the correct action on error - which might just be returning the error to the caller, but don't just ignore it and carry on as if everything was OK.  And make sure things are closed that need to be closed.

[Brian Candler]

On Sunday, 21 March 2021 at 11:42:37 UTC Brian Candler wrote:

Also: somebody has to be responsible for closing the response.  You're not explicitly returning an error from SendNFInstanceRegistration() to HandleNFInstanceRegistration(), so the only way you can indicate that the body is invalid is to return nil as the response pointer.

Sorry, I got that the wrong way round.  SendNFInstanceRegistration calls HandleNFInstanceRegistration, and you do return an error there.

However, PutAndPatch calls SendNFInstanceRegistration, and you don't return an error.  The caller then checks resp.StatusCode, but doesn't account for the fact that there might have been some error, and so resp is nil.

I would suggest that instead of returning a resp, you return bool "ok" which just says whether the request was successful or not.

[Van Fury]

Hi,

I have found the problem to the double PUT message being sent and have solve it.

Thanks for the help.

A lot of lesson learnt from this exercise.

[Brian Candler]

Great - glad it's solved.  I was only going to suggest you insert more logging calls all over the place until you find the problem :-)