[ECO-4687][Protocol-2] Fix duplicate outgoing ATTACH msg

[sacOO7]

Main issue was related to the fact that, every msg that needs to be sent is queued first in the global queue and then sent one by one. Since, protocol 2 supports sending force ATTACH on every connected msg, if there is existing ATTACH in queue, both of the ATTACH will be sent on CONNECTED -> https://github.com/ably/specification/pull/193/files

Fixed ably-ruby implementation responsible for sending duplicate ATTACH message.

1. Send attach only when connection state is CONNECTED
2. Do not queue the attach message, instead send immediately on connected
3. Removed use of emitter pattern while clearing attach/detach timers (see Improper use of EventEmitter [violation of RTC10] #413)

Link to internal discussion
https://ably-real-time.slack.com/archives/C8SPU4589/p1718275991214299?thread_ts=1718115075.720879&cid=C8SPU4589 ( check for last few messages regarding decision taken )

PS. It will be easy to understand changes by going through each commit.

[sacOO7]

I have updated implementation for sending ATTACH and DETACH msg to be separate as a part of 8b9b556.
Earlier, most of the code was DETACH specific and had nothing to do with sending ATTACH message.

[lawrence-forooghian]

I've tried taking a look at this PR but it's quite complicated and I don't really understand what's going on. Might be best to have a call about it.

[lawrence-forooghian]

Recapping our call:

• let's move the connection state check to #attach per RTL4i, then we won't need this new force_attach flag
• the final commit is just a refactor; splits the attach and detach logic (which deviated further in the first commit) into separate methods instead of a shared method
• I asked whether we need some new tests? Sachin said he'd look into it
• I still don't understand the notify_state_change method, or what it has to do with Improper use of EventEmitter [violation of RTC10] #413, or what Improper use of EventEmitter [violation of RTC10] #413 has to do with this issue; we need to have another conversation about that

[sacOO7]

Recapping our call:

• let's move the connection state check to #attach per RTL4i, then we won't need this new force_attach flag
• the final commit is just a refactor; splits the attach and detach logic (which deviated further in the first commit) into separate methods instead of a shared method
• I asked whether we need some new tests? Sachin said he'd look into it
• I still don't understand the notify_state_change method, or what it has to do with Improper use of EventEmitter [violation of RTC10] #413, or what Improper use of EventEmitter [violation of RTC10] #413 has to do with this issue; we need to have another conversation about that

• I have removed use of forced_attach from the code and added connected check while calling attach explicitly.
• Added test to make sure no ATTACH/ATTACHED duplicates SENT/RECEIVED -> 47c55d7
• About notify_state_change, it is introduced to replace ->

  ably-ruby/lib/ably/realtime/channel/channel_manager.rb

  Lines 228 to 231 in 7f156b7

  	channel.once_state_changed do
  	@pending_state_change_timer.cancel if @pending_state_change_timer
  	@pending_state_change_timer = nil
  	end

  .
• As, you can see

        channel.once_state_changed do
          @pending_state_change_timer.cancel if @pending_state_change_timer
          @pending_state_change_timer = nil
        end

is registering a callback on emitter class, same is used to register callbacks externally
e.g.

    channel.attach do
       # does some stuff for longer time
    end

• All of these callbacks gets executed in the order they are registered. You can see the callback array ->
  

  ably-ruby/lib/ably/modules/event_emitter.rb

  Line 162 in 7f156b7

  callbacks_any << proc_block

  and it gets executed in a for loop ->

  ably-ruby/lib/ably/modules/event_emitter.rb

  Line 91 in 7f156b7

  [callbacks_any, callbacks[callbacks_event_coerced(event_name)]].each do |callback_arr|

• So, if we first register a external callback that does work for long time, the callback responsible for cancelling timer won't execute. Hence, it will put channel into suspended state.
• This is a one of the problem described as per RTC10 ( do read related spec )
• So, this is a small fix done to avoid sending duplicate ATTACH message, since getting into suspended state again triggers reattach mechanism as per RTL4f
  

  ably-ruby/lib/ably/realtime/channel/channel_state_machine.rb

  Lines 63 to 65 in 7f156b7

  	after_transition(to: [:suspended]) do |channel, current_transition|
  	channel.manager.start_attach_from_suspended_timer
  	end