Implemented sendAsync for js commands with a callback #321

tiabc · 2017-09-14T21:12:13Z

Geth js commands coming through jail with a callback will now be executed truly asynchronously blocking jail only when an actual interaction with VM is performed.

Technically, it registers a new handler jeth.sendAsync which executes functions with callbacks asynchronously.

Changes include:

Send and SendAsync now use cell.VM instead of otto.Otto providing proper locking.
Unmarshalling in ExecuionPolicy.ExecuteWithClient is now done into var result interface{} instead of var result json.RawMessage because test case 0 of TestJailWhisper failed providing byte codes instead of 5.0.
Due to the asynchronous nature of web3 calls new weird timeouts in tests have been introduced. They may fail sometimes but I gave up trying to implement a more reliable and readable solution.

The only failing test is TestJailWhisper. Refer to #336 for explanations.

divan · 2017-09-14T22:00:28Z

geth/jail/execution_policy.go

 		} else {
-			resp = newErrorResponse(call.Otto, -32603, err.Error(), &req.ID).Object()
+			resp = newErrorResponse(-32603, err.Error(), &req.ID)


Maybe move this -32603 constant to constant definition, similar to https://github.com/status-im/status-go/blob/develop/geth/rpc/call_raw.go#L13:

const ( ... errInvalidMessageCode = -32700 // from go-ethereum/rpc/errors.go )

divan · 2017-09-14T22:06:41Z

geth/jail/execution_policy.go

 	}
-
-	return ""
+	msgIDStr, err := msgID.ToString()


There is also a handy (*otto.Value).String() method which returns empty string if there is an error.
https://github.com/status-im/status-go/blob/develop/vendor/github.com/robertkrimen/otto/value.go#L384

1. Encapsulated error code -32603 into newErrorResopnse 2. Shortened a conversion to string

# Conflicts: # geth/jail/execution_policy.go # geth/jail/jail.go

…json.RawMessage

divan · 2017-09-17T18:27:54Z

geth/jail/execution_policy.go

@@ -32,26 +31,25 @@ func NewExecutionPolicy(
 }

 // Execute handles the execution of a RPC request and routes appropriately to either a local or remote ethereum node.


and routes appropriately to either a local or remote ethereum node should be removed, this function doesn't do routing anymore.

adambabik · 2017-09-17T18:28:52Z

Due to the asynchronous nature of web3 calls new weird timeouts in tests have been introduced. They may fail sometimes but I gave up trying to implement a more reliable and readable solution.

I think the only way we can fix it is by removing jail cells or creating a new node instance in every tests instead of reusing it. I checked web3.js and some commands (like creating a contract) register filters and do not provide a way to cancel them.

divan · 2017-09-17T18:29:38Z

geth/jail/execution_policy.go

 	}

 	client := ep.nodeManager.RPCClient()

-	return ep.executeWithClient(client, req, call)
+	return ep.executeWithClient(client, vm, req)


Actually, after moving routing into rpc package, there is no need to have execute with client function. Either inline its code here, or move client := ep.nodeManager.RPCClient() to that function.

I guess that make sense, do we still need ExecutionPolicy?

Can we refactor this out or rename?

I think ExecutionPolicy refactoring is out of scope. Added to #200.

influx6 · 2017-09-17T18:30:10Z

geth/jail/handlers.go

+	cell := cellInt.(*Cell)
+	return func(call otto.FunctionCall) otto.Value {
+		go func() {
+			response := jail.Send(call, cell.VM)


Do we have tests ensuring we are not loosing references anymore?

We've never lost references. These were just race conditions. But we have one decent test anyway:

status-go/geth/jail/jail_test.go

Line 128 in 6800cf8

func (s *JailTestSuite) TestJailRPCAsyncSend() {

influx6

Looks aright

divan · 2017-09-17T18:37:56Z

geth/jail/handlers.go

+	cell := cellInt.(*Cell)
+	return func(call otto.FunctionCall) otto.Value {
+		go func() {
+			response := jail.Send(call, cell.VM)


Doesn't the same comments as on line :92 (in makeSendHandler - Send calls are guaranteed to be only invoked...) applies here? Should not it unlock and lock mutex as well here?

Nope. Moreover, we'll get panic if we do this.
The reason is that we run this in a goroutine and just return otto.UndefinedValue() and this is fast. The cell will be quickly unlocked after these 2 operations and jail.Send execution in the goroutine will start afterwards.

Ok. I'm not sure I completely understand workflow here, but fine for now.

This is an important piece, let's sort it out. I may also turn out to be wrong here and it will be fatal. I'll draw a diagram in a minute.

Something like that:

Woah, this is a cool picture explaining a lot. Let's add it do the documentation doc.

Just one question with calls without callbacks: if a long running operation will be called and another independent call without callback will be executed, we are still fine because we use thread-safe otto (our vm.VM struct)?

Yes, these two will be just run in sequence.

divan · 2017-09-17T18:41:24Z

geth/jail/jail.go

+func newErrorResponseOtto(vm *vm.VM, msg string, id interface{}) otto.Value {
+	// TODO(tiabc): Handle errors.
+	errResp, _ := json.Marshal(newErrorResponse(msg, id))
+	errRespVal, _ := vm.Run("(" + string(errResp) + ")")


This looks a bit cryptic. Why do we call JS to form error?

Shouldnt we assign it into a variable in the script source to be runned?

@divan this is legacy. But it seems that's to get otto.Value out of an arbitrary object.
@influx6 nope, you can run it and get the result into errRespVal.

i see. Understood.

influx6 · 2017-09-17T18:44:22Z

Makefile

@@ -47,7 +47,7 @@ statusgo-ios-simulator-mainnet: xgo
 	@echo "iOS framework cross compilation done (mainnet)."

 ci: mock
-	build/env.sh go test -timeout 10m -v ./geth/api/...
+	build/env.sh go test -timeout 40m -v ./geth/api/...


Didnt we already change this in another PR?

Yes but it's not merged yet.

tiabc · 2017-09-17T18:45:49Z

I think the only way we can fix it is by removing jail cells or creating a new node instance in every tests instead of reusing it. I checked web3.js and some commands (like creating a contract) register filters and do not provide a way to cancel them.

Sorry, @adambabik, didn't get your comment. My comment was about waiting callbacks, are we on the same page?

adambabik

Overall LGTM, but there are some nuances I don't get.

adambabik · 2017-09-17T18:30:16Z

geth/api/backend_jail_test.go

@@ -264,6 +264,9 @@ func (s *BackendTestSuite) TestContractDeployment() {
 		s.FailNow("test timed out")
 	}

+	// Wait until callback is fired and `responseValue` is set. Hacky but simple.


Oh, so was it failing sometimes because of invalid responseValue? I have never noticed that...

Yes it was. It hasn't failed before just because callbacks were fired synchronously. Now that they're really asynchronous, we somehow need to sync assertions with callbacks invocations.

You can refer to this comment to see the whole picture: #321 (comment)

adambabik · 2017-09-17T18:32:22Z

geth/jail/execution_policy.go

-	if err != nil {
-		return nil, err
+func (ep *ExecutionPolicy) executeSendTransaction(vm *vm.VM, req common.RPCCall) (map[string]interface{}, error) {
+	// TODO(tiabc): Move it to the bottom where `result` and `hash` are set.


Can we move it down in this PR? It seems like it can be just returned with all values in return statement.

adambabik · 2017-09-17T18:34:42Z

geth/jail/handlers.go

@@ -66,23 +66,54 @@ func registerHandlers(jail *Jail, cell common.JailCell, chatID string) error {
 	return nil
 }

+// makeAsyncSendHandler returns jeth.sendAsync() handler.
+func makeAsyncSendHandler(jail *Jail, cellInt common.JailCell) func(call otto.FunctionCall) otto.Value {
+	// FIXME(tiabc): Get rid of this.


Hm? Can we get rid of this in this PR? I guess we can just use cell *Cell?

Yes but it better be done during some refactoring as it would introduce a number of unrelated changes. Generally, I'd like to revisit how we use jail cell and its interface.

adambabik · 2017-09-17T18:37:30Z

geth/jail/handlers.go

+		go func() {
+			response := jail.Send(call, cell.VM)
+
+			if fn := call.Argument(1); fn.Class() == "Function" {


Hope it's always a a case params, callback :)

More bulletproof solution could be: fn := call.Argument(call.NumberOfArguments - 1), provided there is a prop which returns a number of arguments.

Nope, send in web3 always has a single argument. sendAsync always has only 2 arguments. If it changes in web3 we'd better receive an error during tests rather than come across some non-obvious behavior.

adambabik · 2017-09-17T18:38:45Z

geth/jail/handlers.go

+	cell := cellInt.(*Cell)
+	return func(call otto.FunctionCall) otto.Value {
+		go func() {
+			response := jail.Send(call, cell.VM)


I wonder if we shouldn't call this outside of a goroutine and return the result... It usually should be undefiend but who knows.

No, we shouldn't. This invocation is actually the main point of this PR which introduced all the other changes.

Oh ok. I get it now after giving it a little bit more thinking :)

adambabik · 2017-09-17T18:44:38Z

geth/jail/handlers.go

+		// method of jail.Cell and the cell is locked during that call. In order to allow jail.Send
+		// to perform any operations on cell.VM and not hang, we need to unlock the mutex and return
+		// it to the previous state afterwards so that the caller didn't panic doing cell.Unlock().
+		cell.Unlock()


So we're bypassing the fact that cell might be blocked by other call? E.g. there is a web3 call which hangs and send command is sent at the same time and this code will allow to do that?

Yep, it's alright and there's even a test for that. The reason is that jail.Send now make calls through cell.VM - the very same VM that is locked upon the initial call. So we need to unlock it. Otherwise, it's locked 2 times during a single jail.Call and hangs.

adambabik · 2017-09-17T18:58:09Z

geth/jail/jail_test.go

@@ -119,6 +125,36 @@ func (s *JailTestSuite) TestFunctionCall() {
 	require.Equal(expectedResponse, response)
 }

+func (s *JailTestSuite) TestJailRPCAsyncSend() {


Shouldn't we make a simple test which also checks if a callback is called? I am not sure what we're testing here as web3.eth.sendTransaction(data, function(){}) would be called without these changes anyway, just in sync way I guess.

It's checked everywhere in a lot of tests, this is why I decided not to include another test.
I'll write a comment what this test is about.

Implemented first version (tests fail)

6800cf8

tiabc added the in progress label Sep 14, 2017

tiabc changed the title ~~Implemented first version (tests fail)~~ Implemented sendAsync for js commands with a callback Sep 14, 2017

tiabc mentioned this pull request Sep 14, 2017

Jail sendAsync: Add method to jail sendAsync with tests #194

Closed

divan reviewed Sep 14, 2017

View reviewed changes

tiabc added 4 commits September 15, 2017 11:48

Improved error handling in jail.Send

9594aff

Fixed TestContractDeployment

650e9c4

Fixed review comments:

5b3c594

1. Encapsulated error code -32603 into newErrorResopnse 2. Shortened a conversion to string

Merge branch 'develop' into feature/send-async-#184

3cc05f8

# Conflicts: # geth/jail/execution_policy.go # geth/jail/jail.go

tiabc removed the in progress label Sep 15, 2017

tiabc added 2 commits September 15, 2017 13:53

Fixed compilation error

9614aa8

Got timeout back to 40m

0f65cc5

tiabc added the in progress label Sep 15, 2017

tiabc added 5 commits September 17, 2017 17:17

Fixed jail.Send hanging

b8ef6f1

Fixed TestJailVMPersistence

37331e0

Fixed TestJailWhisper test 0

cb082a3

Made client.CallContext return an object with "real" data instead of …

716885b

…json.RawMessage

Merge branch 'develop' into feature/send-async-#184

79d599b

tiabc mentioned this pull request Sep 17, 2017

Fix TestJailWhisper #336

Closed

tiabc removed the in progress label Sep 17, 2017

divan reviewed Sep 17, 2017

View reviewed changes

influx6 reviewed Sep 17, 2017

View reviewed changes

influx6 approved these changes Sep 17, 2017

View reviewed changes

divan reviewed Sep 17, 2017

View reviewed changes

influx6 reviewed Sep 17, 2017

View reviewed changes

adambabik reviewed Sep 17, 2017

View reviewed changes

Fixed review comments

7288917

adambabik approved these changes Sep 17, 2017

View reviewed changes

divan approved these changes Sep 18, 2017

View reviewed changes

tiabc merged commit 5f19c9c into develop Sep 18, 2017

tiabc mentioned this pull request Sep 18, 2017

Fixed jail hanging #303

Closed

adambabik deleted the feature/send-async-#184 branch December 27, 2019 10:02

		@@ -32,26 +31,25 @@ func NewExecutionPolicy(
		}

		// Execute handles the execution of a RPC request and routes appropriately to either a local or remote ethereum node.

Implemented sendAsync for js commands with a callback #321

Implemented sendAsync for js commands with a callback #321

Conversation

tiabc commented Sep 14, 2017 • edited Loading

Choose a reason for hiding this comment

divan Sep 14, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adambabik commented Sep 17, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

influx6 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tiabc commented Sep 17, 2017 • edited Loading

adambabik left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tiabc commented Sep 14, 2017 •

edited

Loading

divan Sep 14, 2017 •

edited

Loading

tiabc commented Sep 17, 2017 •

edited

Loading