refactor: Added protocol callback error handling #393

soluchok · 2019-10-02T11:27:02Z

ability to consume the result of the callback function

Closes: #242

Signed-off-by: Andrii Soluk isoluchok@gmail.com

codecov · 2019-10-02T11:32:44Z

Codecov Report

Merging #393 into master will increase coverage by 0.05%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master     #393      +/-   ##
==========================================
+ Coverage   89.63%   89.69%   +0.05%     
==========================================
  Files          56       56              
  Lines        3271     3289      +18     
==========================================
+ Hits         2932     2950      +18     
  Misses        183      183              
  Partials      156      156

Impacted Files	Coverage Δ
pkg/didcomm/protocol/didexchange/service.go	`94.84% <100%> (+0.43%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 238da0b...8c5ff82. Read the comment docs.

troyronda · 2019-10-02T11:37:24Z

@soluchok normal semantic PR prefixes: https://github.com/commitizen/conventional-commit-types/blob/v2.2.0/index.json (from https://github.com/probot/semantic-pull-requests)

ie refactor not refactoring

rolsonquadras · 2019-10-02T13:48:58Z

@soluchok issue #242 is a feature issue. The PR title says "refactor". Should the title be updated with "feat" ?

rolsonquadras · 2019-10-02T13:54:25Z

pkg/didcomm/protocol/didexchange/service_test.go

+	msg := &dispatcher.DIDCommCallback{}
+	err = msg.Done(dispatcher.Result{Err: err})
+	require.NoError(t, err)
+	e.Callback(msg)


Can you please add a test case to validate the error from callback processing ? i.e, consumer calls the callback and validates the error from the callback.

Done, I've modified the existing one
https://github.com/hyperledger/aries-framework-go/pull/393/files#diff-36ac39bebf32030fd7dff6f12aea8d7eR797

troyronda · 2019-10-02T14:49:59Z

@rolsonquadras @soluchok "feat: Added error handling" will be a meaningless entry in the change log.

pkg/didcomm/dispatcher/api_test.go

rolsonquadras · 2019-10-02T15:48:34Z

@soluchok issue #242 is a feature issue. The PR title says "refactor". Should the title be updated with "feat" ?

Any specific reason to revert back to "refactor". Also the commit message doesn't have semantic prefix.

soluchok · 2019-10-02T15:59:36Z

@soluchok issue #242 is a feature issue. The PR title says "refactor". Should the title be updated with "feat" ?

Any specific reason to revert back to "refactor". Also the commit message doesn't have semantic prefix.

Ok, let's decide is it refactor or feat? @troyronda @rolsonquadras
and I will change comment and title

rolsonquadras · 2019-10-02T17:15:31Z

@soluchok issue #242 is a feature issue. The PR title says "refactor". Should the title be updated with "feat" ?

Any specific reason to revert back to "refactor". Also the commit message doesn't have semantic prefix.

Ok, let's decide is it refactor or feat? @troyronda @rolsonquadras
and I will change comment and title

My bad, missed @troyronda 's comment.

pkg/didcomm/protocol/didexchange/service_test.go

pkg/didcomm/dispatcher/api.go

pkg/didcomm/dispatcher/api_test.go

pkg/didcomm/dispatcher/api.go

pkg/didcomm/protocol/didexchange/service_test.go

pkg/didcomm/dispatcher/api.go

pkg/didcomm/protocol/didexchange/service_test.go

pkg/didcomm/protocol/didexchange/service.go

troyronda · 2019-10-07T16:58:01Z

This PR may need a discussion. I chatted with @rolsonquadras about the approach and he plans to write something up.

Briefly my thoughts were:

Callback is currently handling two scenarios (Client wants to signal 'Continue with Protocol' vs 'Fail/Stop Protocol'). Suggest these be separated (e.g., Fail doesn't need an error channel).
For a client to have handle (multiple) async scenarios, it would have been logical for that client to pass-in a channel for receiving error events. In this way they could setup a for { select {} } for all event handling.
I still don't fully understand the done scenario (and the code for it seemed complex).
For a client that handles sync scenarios, they would probably wrap the callback error channel into a blocking call ( ending up with err := continueWrapper(...) )

rolsonquadras · 2019-10-07T19:55:28Z

This PR may need a discussion. I chatted with @rolsonquadras about the approach and he plans to write something up.

The current consumer code looks something like follows without error handling (master branch).

	actionCh := make(chan dispatcher.DIDCommAction)
	err := svc.RegisterActionEvent(actionCh)
	for {
		select {
			case e := <-c.actionCh:
				e.Continue() 	// e.Callback() is changed to e.Continue  #437
				// TODO handle error from callback
		}
	}

With the error handling code added in this PR, the consumer code will look like below.

	actionCh := make(chan dispatcher.DIDCommAction)
	err := svc.RegisterActionEvent(actionCh)
	for {
		select {
			case e := <-c.actionCh:
				msg := dispatcher.NewDIDCommCallback()
				for {
					select {
					case res := <-msg.Result():
						// process error
				}()
				e.Continue(msg)
		}
	}

This would block the main select(actionCh) and other actionEvents will buffer. This would be similar to sync call err := e.Continue().

Briefly my thoughts were:

Callback is currently handling two scenarios (Client wants to signal 'Continue with Protocol' vs 'Fail/Stop Protocol'). Suggest these be separated (e.g., Fail doesn't need an error channel).

This has been handled through #436 and #437.

For a client to have handle (multiple) async scenarios, it would have been logical for that client to pass-in a channel for receiving error events. In this way they could setup a for { select {} } for all event handling.

	actionCh := make(chan dispatcher.DIDCommAction)
	err := svc.RegisterActionEvent(actionCh)
	errorCh := make(chan <some_type)
	
	for {
		select {
			case e := <-c.actionCh:
				e.Continue(errorCh)
			case e := <-errorCh:
				// process the error
				// correlate based on ConnectionID - https://github.com/hyperledger/aries-framework-go/blob/master/pkg/didcomm/protocol/didexchange/service.go#L254
		}
	}

@troyronda @soluchok

soluchok · 2019-10-07T20:07:58Z

@rolsonquadras

With the error handling code added in this PR, the consumer code will look like below.

	actionCh := make(chan dispatcher.DIDCommAction)
	err := svc.RegisterActionEvent(actionCh)
	for {
		select {
		case e := <-c.actionCh:
			msg := dispatcher.NewDIDCommCallback()
                        e.Continue(msg)
                        go func(m *dispatcher.DIDCommCallback){
			    if  res := <-m.Result(); res.Err != nil {
				e.Stop(res.Err)
			    }
                        }(msg)
		}
	}

rolsonquadras · 2019-10-07T20:28:13Z

@rolsonquadras

With the error handling code added in this PR, the consumer code will look like below.

	actionCh := make(chan dispatcher.DIDCommAction)
	err := svc.RegisterActionEvent(actionCh)
	for {
		select {
		case e := <-c.actionCh:
			msg := dispatcher.NewDIDCommCallback()
                        e.Continue(msg)
                        go func(m *dispatcher.DIDCommCallback){
			    if  res := <-m.Result(); res.Err != nil {
				e.Stop(res.Err)
			    }
                        }(msg)
		}
	}

Consumer will either invoke either Continue() or Stop(). For Stop(), consumer won't send back the error received from the callback. It's the error from action processing.

PR updated

troyronda · 2019-10-08T11:19:05Z

pkg/didcomm/dispatcher/api.go

 // DIDCommAction message type to pass events in go channels.
 type DIDCommAction struct {
 	// DIDComm message
 	Message DIDCommMsg
 	// Continue function to be called by the consumer for further processing the message.
-	Continue func()
+	Continue func(chan<- ActionError)


It would be nice if we could avoid having to pas a nil argument when the caller doesn’t care about the error chan. I wonder if we should use a variadic pattern?

In my opinion, better to pass a clear argument, it's much easier to work with it. But options are not a bad approach, especially for callback (probably it will require one more option in the future)

I like Rolson's snippet towards the end of this comment better. It would be simpler from a user perspective if the error message has a Retry() function or something similar.

It has res.Action.Continue() if you need retry it

troyronda · 2019-10-08T11:21:31Z

pkg/didcomm/protocol/didexchange/service_test.go

+		e.Continue(eChan)
+		select {
+		case res := <-eChan:
+			require.EqualError(t, res.Err, "document for the id doesn't exists in the database: data not found")


it would be good to have a test case showing correlation.

Signed-off-by: Andrii Soluk <isoluchok@gmail.com>

soluchok changed the title ~~refactoring: Added error handling~~ refactor: Added error handling Oct 2, 2019

fqutishat requested review from rolsonquadras, sandrask, troyronda, fqutishat and bstasyszyn October 2, 2019 13:47

soluchok changed the title ~~refactor: Added error handling~~ feat: Added error handling Oct 2, 2019

rolsonquadras reviewed Oct 2, 2019

View reviewed changes

soluchok changed the title ~~feat: Added error handling~~ refactor: Added callback error handling Oct 2, 2019

troyronda changed the title ~~refactor: Added callback error handling~~ refactor: Added dispatcher callback error handling Oct 2, 2019

troyronda changed the title ~~refactor: Added dispatcher callback error handling~~ refactor: Added protocol callback error handling Oct 2, 2019

troyronda reviewed Oct 2, 2019

View reviewed changes

pkg/didcomm/dispatcher/api_test.go Outdated Show resolved Hide resolved

bstasyszyn approved these changes Oct 4, 2019

View reviewed changes

troyronda requested a review from llorllale October 4, 2019 13:44

fqutishat reviewed Oct 4, 2019

View reviewed changes

pkg/didcomm/protocol/didexchange/service_test.go Outdated Show resolved Hide resolved

llorllale reviewed Oct 4, 2019

View reviewed changes

pkg/didcomm/dispatcher/api.go Outdated Show resolved Hide resolved

llorllale reviewed Oct 4, 2019

View reviewed changes

pkg/didcomm/dispatcher/api_test.go Outdated Show resolved Hide resolved

troyronda previously requested changes Oct 4, 2019

View reviewed changes

rolsonquadras mentioned this pull request Oct 7, 2019

DIDExchange Event - Provide separate API for Continue and Fail instead of Callback #436

Closed

rolsonquadras mentioned this pull request Oct 7, 2019

Service - Retry logic #242

Open

troyronda self-requested a review October 8, 2019 11:12

troyronda reviewed Oct 8, 2019

View reviewed changes

troyronda requested review from bstasyszyn, fqutishat, llorllale and rolsonquadras October 8, 2019 11:19

troyronda reviewed Oct 8, 2019

View reviewed changes

refactor: Added protocol callback error handling

8c5ff82

Signed-off-by: Andrii Soluk <isoluchok@gmail.com>

troyronda added the paused label Oct 8, 2019

soluchok closed this Oct 9, 2019

soluchok deleted the issue-242 branch October 22, 2019 16:17

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor: Added protocol callback error handling #393

refactor: Added protocol callback error handling #393

soluchok commented Oct 2, 2019

codecov bot commented Oct 2, 2019 •

edited

Loading

troyronda commented Oct 2, 2019

rolsonquadras commented Oct 2, 2019

rolsonquadras Oct 2, 2019

soluchok Oct 2, 2019 •

edited

Loading

troyronda commented Oct 2, 2019

rolsonquadras commented Oct 2, 2019

soluchok commented Oct 2, 2019

rolsonquadras commented Oct 2, 2019

troyronda commented Oct 7, 2019 •

edited

Loading

rolsonquadras commented Oct 7, 2019

soluchok commented Oct 7, 2019 •

edited

Loading

rolsonquadras commented Oct 7, 2019

troyronda Oct 8, 2019

soluchok Oct 8, 2019

soluchok Oct 8, 2019

llorllale Oct 8, 2019

soluchok Oct 8, 2019

troyronda Oct 8, 2019

refactor: Added protocol callback error handling #393

refactor: Added protocol callback error handling #393

Conversation

soluchok commented Oct 2, 2019

codecov bot commented Oct 2, 2019 • edited Loading

Codecov Report

troyronda commented Oct 2, 2019

rolsonquadras commented Oct 2, 2019

rolsonquadras Oct 2, 2019

Choose a reason for hiding this comment

soluchok Oct 2, 2019 • edited Loading

Choose a reason for hiding this comment

troyronda commented Oct 2, 2019

rolsonquadras commented Oct 2, 2019

soluchok commented Oct 2, 2019

rolsonquadras commented Oct 2, 2019

troyronda commented Oct 7, 2019 • edited Loading

rolsonquadras commented Oct 7, 2019

soluchok commented Oct 7, 2019 • edited Loading

rolsonquadras commented Oct 7, 2019

troyronda Oct 8, 2019

Choose a reason for hiding this comment

soluchok Oct 8, 2019

Choose a reason for hiding this comment

soluchok Oct 8, 2019

Choose a reason for hiding this comment

llorllale Oct 8, 2019

Choose a reason for hiding this comment

soluchok Oct 8, 2019

Choose a reason for hiding this comment

troyronda Oct 8, 2019

Choose a reason for hiding this comment

codecov bot commented Oct 2, 2019 •

edited

Loading

soluchok Oct 2, 2019 •

edited

Loading

troyronda commented Oct 7, 2019 •

edited

Loading

soluchok commented Oct 7, 2019 •

edited

Loading