Rework RpcServer.ProcessAsync to be non-terminal ASP.NET Middleware

[devhawk]

This will allow neo express to use RpcServer in an HTTP pipeline that RpcServer doesn't create

[shargon]

protected and extend the class?

[devhawk]

protected and extend the class?

If we're going to do this, we should provide a mechanism to inject steps into the http pipeline that StartRpcServer creates. Maybe we should separate out the pipeline host code from the rest of RpcServer and make RpcServer a standard ASP.NET Middleware class? Should be pretty straightfoward since RpcServer already plugs into the http pipeline via app.run. WOuldn't take much to make it work with App.use

[erikzhang]

If we're going to do this, we should provide a mechanism to inject steps into the http pipeline that StartRpcServer creates. Maybe we should separate out the pipeline host code from the rest of RpcServer and make RpcServer a standard ASP.NET Middleware class? Should be pretty straightfoward since RpcServer already plugs into the http pipeline via app.run. WOuldn't take much to make it work with App.use

It sounds good.

[devhawk]

If we're going to do this, we should provide a mechanism to inject steps into the http pipeline that StartRpcServer creates. Maybe we should separate out the pipeline host code from the rest of RpcServer and make RpcServer a standard ASP.NET Middleware class? Should be pretty straightfoward since RpcServer already plugs into the http pipeline via app.run. WOuldn't take much to make it work with App.use

It sounds good.

I'll work on this later this week.

[devhawk]

OK, here's what I did:

• modified ProcessRequest to return null for invalid requests (missing id/method/params or non-array params) instead of returning JsonRpc Error response
• added next RequestDelegate parameter to ProcessAsync. next parameter invoked if there is not a valid response generated from the request instead of returning a JsonRpc Error response
• added InvalidRequestAsync method to return JsonRpc error response by default if ProcessAsync method doesn't handle the request. This provides nearly identical invalid response behavior to current RpcServer (error codes are different in certain scenarios)
• Modified StartRpcServer to Use ProcessRequest and Run InvalidRequestAsync.

These changes will allow neo-express to use ProcessAsync in its own WebHost instance (where I can compose the JsonRpc server behavior with express-specific non-JsonRpc functionality) w/o modifying the behavior of RpcServer when it's used a standard plugin for neo-cli

[devhawk]

Ping @erikzhang @shargon

[erikzhang]

modified ProcessRequest to return null for invalid requests (missing id/method/params or non-array params) instead of returning JsonRpc Error response

I don't understand why it should be modified like this.

[devhawk]

FYI, with 3.2 release on the horizon, I will get this PR updated shortly

[devhawk]

modified ProcessRequest to return null for invalid requests (missing id/method/params or non-array params) instead of returning JsonRpc Error response

I don't understand why it should be modified like this.

So that ProcessAsync can tell if the request should be passed thru to the next delegate in the ASP.NET middleware pipeline. This let's NeoExpress create its own kestrel service that also composes RpcServer. Express code will look something like this:

.Configure(app =>
{
    app.UseResponseCompression();
    app.Use(next => context => rpcServer.ProcessAsync(context, next));
    app.Run(NeoExpressFunctionality);
})

This way, rpcServer always gets first crack at the request so that anything it considers worthy of a response is processed immediately and never gets passed down the pipeline.

[devhawk]

One thing I just realized about this PR is how it handles multiple JSON RPC requests in an array. Currently, it simply filters out null responses from ProcessRequest. I don't think that's the right choice. Instead, if ANY request in the array gets a valid response, then any invalid responses should get an error return not get filtered out. Will update

Never mind, just realized that the original code is also filtering.

[devhawk]

cc @erikzhang @shargon I am trying to get this merged for 3.2

[erikzhang]

I think we should use InvalidRequestAsync first and then run ProcessAsync as the terminal middleware. Then InvalidRequestAsync can catch the exceptions in ProcessAsync.

[devhawk]

If we're going to make ProcessAsync the terminal middleware, then I think we should simply go back to the original commit in this PR and make ProcessAsync public.

I split InvalidRequestAsync out from ProcessAsync so Express could insert its middleware in the ASP.NET Pipeline after ProcessAsync. If ProcessAsync is terminal middleware, then Express has no choice but to put its middleware before ProcessAsync. Compatibility with RpcServer is a high priority for Express, so I wanted to give ProcessAsync the highest priority position in the ASP.NET pipeline. But that doesn't work if ProcessAsync can only be used as terminal middleware.

Honestly, I don't care which approach we take. Simply making ProcessAsync public is smaller and has no logic changes. Changing ProcessAsync to be non-terminal means for less code for Express. Either approach works for me.

[devhawk]

I've renamed this PR to reflect the code changes better. I also opened a new PR #701 that only makes the visibility change.

As per my last comment, either approach is fine by me. But I need either this PR or #701 to be accepted in order to enable the new features we are adding to Express.