Hi Brian,
Thanks for the reply. I intentionally left the error checks out so as to
make the code short.
Also using the Client.Do was an error and its suppose to be
transport.Client.Do.
Also in the main() does other stuff after the PUT and PATCH messages but
does not have any function that is
using the data concurrently.
Thanks for the advice on the code structure. I will restructure my code to
the PuTAndPatch().
BR
Van
On Saturday, March 20, 2021 at 12:04:33 PM UTC+2 Brian Candler wrote:
> *> 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 <https://blog.golang.org/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
> <https://www.youtube.com/watch?v=5zXAHh5tJqQ>.
>
> 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 <https://blog.golang.org/race-detector> to help find such
> problems.
>
> HTH,
>
> Brian.
>
--
You received this message because you are subscribed to the Google Groups
"golang-nuts" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to golang-nuts+unsubscr...@googlegroups.com.
To view this discussion on the web visit
https://groups.google.com/d/msgid/golang-nuts/7b371be7-9047-4478-8a2b-81eddada66b2n%40googlegroups.com.
