address unbounded channel in signal client #70
Conversation
| @@ -903,11 +893,11 @@ async fn con_manage_connection( | |||
| let msg = msg.into_data(); | |||
| match dbg_err!(wire::Wire::decode(&msg)) { | |||
| wire::Wire::DemoV1 { rem_pub } => { | |||
| recv_cb(SignalMsg::Demo { rem_pub }); | |||
| let _ = msg_send.send(SignalMsg::Demo { rem_pub }).await; | |||
There was a problem hiding this comment.
The error is not expected to be important here?
| @@ -982,13 +966,13 @@ async fn decode_fwd( | |||
|
|
|||
| match msg { | |||
| wire::FwdInnerV1::Offer { offer, .. } => { | |||
| recv_cb(SignalMsg::Offer { rem_pub, offer }); | |||
| let _ = msg_send.send(SignalMsg::Offer { rem_pub, offer }).await; | |||
There was a problem hiding this comment.
Same question here, if these fail should the cli exit, report or swallow the error?
There was a problem hiding this comment.
@ThetaSinner - This is one of those shutdown cases... i.e. the only time you'd get an error here is if the listener has been dropped, in which case I think it would just cause confusion if we were to report it into the tracing or something. BTW, this is actually production code, so not sure if that alters your impression of it, please let me know if you still think I should change something!
There was a problem hiding this comment.
Shutdown is a very weak muscle for us. Our software tends to just fall into a cascade of errors when it's time to be done. We didn't add clean shutdown to Kitsune, and now it's hard to stop some networks while keeping others running. We still don't have a solution for that, because we don't have time to refactor it now.
Is it definitely the case that the signal server shutdown will always be all-or-nothing, where we never need to shut down some parts and leave others running? If so, maybe ignoring errors can be justified. I don't know enough about this program to make that call.
In my ideal world we would always handle shutdown properly, with a task manager that understands the proper shutdown order (task-motel can do this). In retrospect it would have been the right thing to do for Holochain/Kitsune. It allows a thing to grow beyond its initial design. Also I think there is some wisdom in understanding what it takes to properly dismantle something that was built (and actually embodying that in code instead of it just living in our heads), it tends to help us understand the building part better which might make the thing built better too.
Of course the relevance here is that if we had proper shutdown, then this error could be handled as an error and it wouldn't be confusing (even if in reality it never happened). Handling errors like this as actual errors proves that we have shutdown under control.
Not requiring a change if the signal server really is simple enough to warrant laissez-faire shutdown, just want to convey this principle and see if it sparks anything.
github-merge-queue
bot
removed this pull request from the merge queue due to no response for status checks
No description provided.