Throw on received message if the RoC of an all-nodes request has finished #310
Conversation
david-eckardt-sociomantic
requested review from
gavin-norman-sociomantic and
nemanja-boric-sociomantic
| @@ -252,25 +252,29 @@ public struct RequestOnConnSet | |||
|
|
|||
| /*************************************************************************** | |||
|
|
|||
| Decrements the `num_active` counter and returns whether it is now 0. | |||
| Decrements the `num_active` counter, registers `roc` as inactive and | |||
| returns whether it is now 0. | |||
There was a problem hiding this comment.
Your addition to this sentence has changed the meaning of "it" at this point.
| returns whether it is now 0. | ||
|
|
||
| Params: | ||
| roc = the `RequestOnConn` instance to register as inactive |
There was a problem hiding this comment.
Maybe "the RequestOnConn instance that has finished" is a bit clearer?
| // avoid potentially introducing a bug if there is an unobvious reason | ||
| // why it isn't done. | ||
| request_on_conn.active = false; | ||
| // TODO: Consider `this.all_nodes.remove(roc)` instead of using the |
There was a problem hiding this comment.
Actually, this would be better as an issue, rather than a code comment. We'll probably never think of it again, if it's left as a comment deep in the code like this.
| // using the `active` flag. Not doing this now for a patch release to | ||
| // avoid potentially introducing a bug if there is an unobvious reason | ||
| // why it isn't done. | ||
| request_on_conn.active = false; |
There was a problem hiding this comment.
What does request_on_conn refer to? Looks like it's intended to be an argument, but the function has no arguments.
src/swarm/neo/client/RequestSet.d
Outdated
| @@ -439,7 +439,8 @@ public final class RequestSet: IRequestSet | |||
| } | |||
| body | |||
| { | |||
| if ( this.getRequestOnConnForNode(connection.remote_address) !is null ) | |||
| if ( this.getRequestOnConnForNode(connection.remote_address, | |||
There was a problem hiding this comment.
Oh this is a weird case! This should be impossible, right? (If so, this is more like a verify than a protocol error.)
There was a problem hiding this comment.
Passing the exception here is merely for the sake of the arguments of getRequestOnConnForNode. I can add a null default for the exception parameter.
david-eckardt-sociomantic
force-pushed
the
roc-finished
branch
from
701c24e to
2b6afcd
Compare
If a `RequestOnConn` is registered in the `RequestOnConnSet`, make it possible to determine if `RequestOnConnSet.finished` has been called with it.
…shed Fixes sociomantic-tsunami#309. When a RoC of a multi-node request finishes, it RoC stays registered in the `RequestOnConnSet` until all other RoCs have finished, too. During that time the client can receive a message for this request through this connection. It then looks the RoC up in the RoCset, finds it and calls `RequestOnConn.setReceivedPayload`, which attempts to resume the fiber - but the fiber has terminated so it cannot be resumed, and an assertion fails. This is the case of a protocol error, the client has finished handling the request while the node is still sending messages. Handle it by tracking which of the RoCs in the RoCset have finished and throwing `ProtocolError` when attempting to use one.
david-eckardt-sociomantic
force-pushed
the
roc-finished
branch
from
2b6afcd to
7403905
Compare
|
Updated, addressing @gavin-norman-sociomantic's comments. Added issue #311. |
In several situations, including a node shutdown and client/node protocol mismatch, the client can receive a message for a request-on-conn whose fiber has already terminated. Throw a protocol error to initiate a shutdown to prevent
RequestOnConn.setReceivedPayloadfrom attempting to resume the terminated fiber.Fixes #309.