fix-action-id

[mind-ar]

added validation for actionID in command response
fixes #14
fixes #22

[mind-ar]

hi, I was finally able to solve the command collision problem without adding an event broker: added a mutex in the "send" and "requestList" functions.
you can reproduce the command collision with the following example:

	go func() {
		for {
			time.Sleep(300 * time.Millisecond)
			if err := ami.Ping(ctx, amiSocket, ""); err != nil {
				log.Error().Err(err).Msg("Error en Ping")
			}
			log.Debug().Msg("Ping Ok")

		}
	}()
	go func() {
		for {
			time.Sleep(200 * time.Millisecond)
			if err := ami.Ping(ctx, amiSocket, ""); err != nil {
				log.Error().Err(err).Msg("Error en Ping")
			}
			log.Debug().Msg("Ping Ok")

		}
	}()

[heltonmarx]

The goami connection is single threaded.

If you reuse a single connection between multiple threads and commands it will block other threads until one thread complete with connection.

So, the easy solution is create a separate goami connection to each thread, that could overhead the asterisk server.

The hard solution it's create a connection pool.

I believe that what we have with your implementation partly fix that, but I'd like to suggest this Object Pool Design Pattern.

keeping the fix in requestList method:

responses := make([]Response, 0)
for {
  response, err := read(ctx, client)
  if err != nil {
    return nil, err
  }
  if response.Get("ActionID") != id {
    continue
  }
  e := response.Get("Event")
  r := response.Get("Response")
  if e == event {
    responses = append(responses, response)
   } else if e == complete || r != "" && r != "Success" {
     break
  }
}
return responses, nil

Suggestion:

Modify your PR to just to fix the actionID (like described in the title).
And in this mean create another patch with connection pool only.

[mind-ar]

Hi, thanks for your time!
i've followed your suggestion, and reverted the last change (mutex).
i will create a PR with connection pool for reviewing

[mind-ar]

Hi there, let me know if any correction to this PR is needed.
thanks!

[heltonmarx]

why a looping here ?

The ActionId, in case of included in the command, must have to be in the response.
I'd like to suggest returning a known error to be thread outside the library, for example:

var (
  // ErrInvalidResponseActionID occurss when the response doesn't have the same action id
  ErrInvalidResponseActionID = errors.New("invalid response action id")
)

...
        response, err := read(ctx, client)
        if err != nil {
                return nil, err
        }
        if id == "" || id == response.Get("ActionID") {
                return response, err
        }
        return nil, ErrInvalidResponseActionID

[mind-ar]

the AMI interface is asynchronous. the response returned by the "read" function may not be the response to the executed command, but some asynchronous event returned. An example of this is the Logout command, when you run it you will receive two messages from AMI, the response to the command and a "logout" event, depending on the order in which the messages arrives will fail.
the only way I could think of to avoid this is with a loop, waiting for the response to the actionid, but it makes the actionID mandatory