ergo-services / ergo

An actor-based Framework with network transparency for creating event-driven architecture in Golang. Inspired by Erlang. Zero dependencies.
https://docs.ergo.services
MIT License
3.7k stars 143 forks source link

Can Goroutine not be created to handle ServerProcess.handleXXXMessage(...) ? #135

Closed leonlee2013 closed 1 year ago

leonlee2013 commented 1 year ago

go test all passed

Benefits:

  1. ServerProcess.handleXXXMessage(...)will not have to be executed in other goroutines, which improves performance;
  2. gen.ServerProcess.Call/.CallWithTimeout will not be restricted to use inServerProcess.handleXXXMessage(...), reducing user errors, thus improving Ergo's ease of use, robustness and genserver's concurrent processing capability.
halturin commented 1 year ago

What happened if I make Call to the other process within HandleXXX callback?

PS it is getting annoying

leonlee2013 commented 1 year ago

It affects the throughput of the ServerProcess (sender).

I just hope Ergo gets better and better, I'm sorry if you are annoyed and I will not try to convince you to continue this discussion with you. Have a good life and work!!!

PS: i appreciate the code you have written, it is of high quality and extremely readable.

sleipnir commented 1 year ago

It affects the throughput of the ServerProcess (sender).

I just hope Ergo gets better and better, I'm sorry if you are annoyed and I will not try to convince you to continue this discussion with you. Have a good life and work!!!

PS: i appreciate the code you have written, it is of high quality and extremely readable.

Hello @leonlee2013 I'm sure your comments and everything you tried to do was intended to help a project that really looked interesting. I'm really sorry for the response you got. The response was certainly quite annoying and I'm sorry you got this back for all your efforts in trying to help.

PS I think at the very least you deserve an apology.

halturin commented 1 year ago

to be clear... I found this disrespectful after all my attempts to explain here https://github.com/ergo-services/ergo/discussions/132 and here https://github.com/ergo-services/ergo/discussions/133. I expected to see more effort to understand the logic of Ergo's and Erlang's implementations because I've seen a lack of knowledge of the details. But after all, I got yet another push of quite weird changes. Just got tired of spending my time on this. Sorry if it looked offensive.

halturin commented 1 year ago

Here is my last try to explain why this approach is absolutely wrong...

Process handles its mailbox and makes calls of HandleXXX callbacks in a separate goroutine to receive reply messages for the Call requests made within that callbacks. While waiting for the reply message, this process can receive many other messages, which should be deferred and handled later in the same order as they were received.

I would strongly recommend diving deeper into the logic before posting a new PR or bug report.

leonlee2013 commented 1 year ago

Process handles its mailbox and makes calls of HandleXXX callbacks in a separate goroutine to receive reply messages for the Call requests made within that callbacks. While waiting for the reply message, this process can receive many other messages, which should be deferred and handled later in the same order as they were received.

Thank you so much for talking to me about this and I did understand the author's intent from the Ergo code, just as you said.

Waiting for a reply message should not block the processing of other messages, but at least let the user choice. I think this is what the Ergo framework should do.

Also Erog requires that a separate goroutine must be a ServerProcess, my idea is that it should be an arbitrary goroutine. I think Ergo should be able to call Call(...) on any Goroutine and that Direct(...) should not be used as a bridge. Like Erlang it is possible to call gen_server:call/2 from a normal process:

 erlang:spawn(fun() ->
   ...  
   gen_server:call(To, Msg)
  ...
  end).  

It can also be called in the handle_call/3 callback of genserver.

handle_call(Msg, From, State) ->
...
   gen_server:call(To, Msg),
...

Showing you the PR code (which really shouldn't be shown to you in this way) is just to show that this is possible.

I have great respect for what you have done. I may not have expressed myself very clearly and I apologise for any distress I may have caused you.

leonlee2013 commented 1 year ago

Hello @leonlee2013 I'm sure your comments and everything you tried to do was intended to help a project that really looked interesting. I'm really sorry for the response you got. The response was certainly quite annoying and I'm sorry you got this back for all your efforts in trying to help.

PS I think at the very least you deserve an apology.

Thank you for your comments! @sleipnir

leonlee2013 commented 1 year ago

I would very much like you to put your mind aside for a moment and take a closer look at the code changes and my thoughts. After all, Ergo needs to grow faster and better by absorbing more people's ideas. If you insist that this idea is wrong, it's fine, just to give you a reference. Anyway, thanks for all your replies!

halturin commented 1 year ago

but at least let the user choice. I think this is what the Ergo framework should do.

that is exactly what Ergo does. It provides enough flexibility for the developers. You may want to create a custom process just implementing gen.ProcessBehavior, there are two methods only

// ProcessBehavior interface contains methods you should implement to make your own process behavior
type ProcessBehavior interface {
    ProcessInit(Process, ...etf.Term) (ProcessState, error)
    ProcessLoop(ProcessState, chan<- bool) string // method which implements control flow of process
}

so that you can implement message handling entirely on your own. Take inspiration from the gen.Server/gen.Supervisor/gen.Application implementation.

leonlee2013 commented 1 year ago
// ProcessBehavior interface contains methods you should implement to make your own process behavior
type ProcessBehavior interface {
  ProcessInit(Process, ...etf.Term) (ProcessState, error)
  ProcessLoop(ProcessState, chan<- bool) string // method which implements control flow of process
}

so that you can implement message handling entirely on your own. Take inspiration from the gen.Server/gen.Supervisor/gen.Application implementation.

I'm very happy that you understood my idea.:) Inheritgen.Server, gen.ServerProcessimplements its own MyServer.ProcessLoop,MyServerProcess.Call/CallWithTimeout/CallRPC Doing so modifies Ergo's code the least and is an option for my solution.

However, whether you agree or not, I still think that modifying the Ergo code is the better option (backwards compatibility):

  1. migrate ServerProcess.CastAfter/Cast/CastRPC to process.CastAfter/Cast/CastRPC for like gen_cast/2;
  2. migrate ServerProcess.Call/CallWithTimeout/CallRPC to process..Call/CallWithTimeout/CallRPCfor like gen_call/2;
  3. remove ServerProcess.callbackWaitReply/waitReply, and handleXXXMessage without creating a new Goroutine to optimize performance.

PS I drew a UML of gen.Process. I really hope Ergo gets better and better.

halturin commented 1 year ago
migrate ServerProcess.CastAfter/Cast/CastRPC to process.CastAfter/Cast/CastRPC for like gen_cast/2;
migrate ServerProcess.Call/CallWithTimeout/CallRPC to process..Call/CallWithTimeout/CallRPC for like gen_call/2;
remove ServerProcess.callbackWaitReply/waitReply, and handleXXXMessage without creating a new Goroutine to optimize performance.

Never ever. Cast and Call messages belong to the gen.Server logic. Please, stop asking me to do this. Just leave it.

If I had made it this way, it would have been the worst design ever.

leonlee2013 commented 1 year ago

ok, thank you very much for your reply.