Skip to content
This repository has been archived by the owner on Nov 16, 2020. It is now read-only.

Improve amqp 1.0 error handling #57

Merged
merged 1 commit into from
May 8, 2019

Conversation

kjnilsson
Copy link
Contributor

@kjnilsson kjnilsson commented May 3, 2019

Firstly avoid the explicit receive on connection to wait for link
credit. Instead we cache any early forward operations until link credit
has been received.This way any early close events are handled by the
existing callbacks.

Also wait and link the connection process until after the session
process has been started. The session process isn't started until the
sasl handshake has completed. If we link before this and sasl
authenticatio fails the shovel will not report the correct error.

Also write the report update earlier in the terminate function of the
worker.

requires: rabbitmq/rabbitmq-amqp1.0#85

Firstly avoid the explicit receive on connection to wait for link
credit. Instead we cache any early forward operations until link credit
has been received.This way any early close events are handled by the
existing callbacks.

Also wait and link the connection process until after the session
process has been started. The session process isn't started until the
sasl handshake has completed. If we link before this and sasl
authenticatio fails the shovel will not report the correct error.

Also write the report update earlier in the terminate function of the
worker.
@kjnilsson kjnilsson requested a review from hairyhum May 3, 2019 16:41
@hairyhum
Copy link
Contributor

hairyhum commented May 3, 2019

I tried to test that with creating a shovel between amqp0.9.1 and amqp1.0 in rabbitmq, while not having access rights to the amqp1.0 vhost. I've got this error:

{shutdown,
    {gen_fsm,sync_send_event,
        [<0.1081.0>,
         {attach,
             #{name => <<"foo_05e2744e4b99365f_sender">>,
               rcv_settle_mode => first,
               role =>
                   {sender,
                       #{address => <<"foo1">>,durable => unsettled_state}},
               snd_settle_mode => unsettled}}]}}

Then I configured permissions for a vhost, but not the queue and I got this error:

{{badmatch,link_timeout},
 [{rabbit_amqp10_shovel,connect,7,
                        [{file,"src/rabbit_amqp10_shovel.erl"},{line,126}]},
  {rabbit_amqp10_shovel,connect_dest,1,
                        [{file,"src/rabbit_amqp10_shovel.erl"},{line,98}]},
  {rabbit_shovel_worker,handle_cast,2,
                        [{file,"src/rabbit_shovel_worker.erl"},{line,71}]},
  {gen_server2,handle_msg,2,[{file,"src/gen_server2.erl"},{line,1056}]},
  {proc_lib,init_p_do_apply,3,[{file,"proc_lib.erl"},{line,249}]}]}

Is this expected behaviour?
Is that the right way to test this issue?

@hairyhum
Copy link
Contributor

hairyhum commented May 3, 2019

I was using changes from rabbitmq/rabbitmq-amqp1.0#86 to set amqp1.0 vhost.

@kjnilsson
Copy link
Contributor Author

kjnilsson commented May 3, 2019 via email

@hairyhum
Copy link
Contributor

hairyhum commented May 6, 2019

@kjnilsson yes, I modified rabbitmq/rabbitmq-amqp1.0#85 with changes from rabbitmq/rabbitmq-amqp1.0#86

@michaelklishin
Copy link
Member

I've noticed two other usability problems while QA'ing this, it's hard to evaluate Shovel AMQP 1.0 improvements in this area without addressing them. Let's merge this and work on improving the test suite and AMQP 1.0 plugin error handling in this area next.

@michaelklishin michaelklishin merged commit 58201f0 into master May 8, 2019
@michaelklishin michaelklishin deleted the better-amqp10-error-handling branch May 8, 2019 05:52
@michaelklishin
Copy link
Member

Backported to v3.7.x.

@michaelklishin michaelklishin added this to the 3.7.15 milestone May 8, 2019
michaelklishin added a commit that referenced this pull request May 8, 2019
Improve amqp 1.0 error handling

(cherry picked from commit 58201f0)
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants