Panic in SDK functions #2

New issue

Closed

opened 2022-12-15 11:25:52 +00:00 by fyrchik · 4 comments

fyrchik commented

2022-12-15 11:25:52 +00:00

(Migrated from github.com)

Currently we panic in many situations:

We forget to set an obligatory parameter.
Some attribute has unexpected value ((netmap.NodeInfo).Price() -- we expect to get it with FromV2 where an error is processed).
Something else?

While I think (1) is ok, others are not IMO. Also in (1) we could provide obligatory parameters explicitly: hard-to-misuse API is the best. Another option is to have New with obligatory parameters and Set methods on it (private structure with public interface).

@alexvanin @KirillovDenis @carpawell @realloc @acid-ant

Currently we panic in many situations: 1. We forget to set an obligatory parameter. 2. Some attribute has unexpected value (`(netmap.NodeInfo).Price()` -- we expect to get it with `FromV2` where an error is processed). 3. Something else? While I think (1) is ok, others are not IMO. Also in (1) we could provide obligatory parameters explicitly: hard-to-misuse API is the best. Another option is to have `New` with obligatory parameters and `Set` methods on it (private structure with public interface). @alexvanin @KirillovDenis @carpawell @realloc @acid-ant

carpawell commented

2022-12-15 11:38:26 +00:00

(Migrated from github.com)

I always thought that libraries should not panic in any "normal" cases. I think it is ok to have an exported error that says "i will not even try to make RPC call, u make stupid things" (for unset obligatory parameters, parameters that lead to dero division, etc) and allow a client to decide if he wants to panic. Otherwise, we force using recover if a client does not want his app to crash ever, which is not a true way in go.

I always thought that libraries should not panic in any "normal" cases. I think it is ok to have an exported error that says "i will not even try to make RPC call, u make stupid things" (for unset obligatory parameters, parameters that lead to dero division, etc) and allow a client to decide if he wants to panic. Otherwise, we force using `recover` if a client _does not want his app to crash ever_, which is not a true way in `go`.

👍 1

carpawell commented

2022-12-15 11:46:10 +00:00

(Migrated from github.com)

Set a parameter whose value is incorrect (see https://github.com/nspcc-dev/neofs-node/pull/2101)

4. Set a parameter whose value is incorrect (see https://github.com/nspcc-dev/neofs-node/pull/2101)

fyrchik commented

2022-12-15 12:13:07 +00:00

(Migrated from github.com)

Here the only reason for panic is insufficiently expressive type system. If you use the code wrong, it is ok to panic, so I don't agree it is not a Go way. But I don't mind returning an error.

Here the only reason for `panic` is insufficiently expressive type system. If you use the code wrong, it is ok to panic, so I don't agree it is not a Go way. But I don't mind returning an error.

fyrchik commented

2023-01-12 09:40:21 +00:00

(Migrated from github.com)

We could panic if client init/dial parameter is invalid.
Let's not panic in the RPCs themselves.

We could panic if client init/dial parameter is invalid. Let's not panic in the RPCs themselves.

👍 1