Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

kernel: only overwrite request prompting_message when there could be a reply to it #411

Merged
merged 2 commits into from
Jun 20, 2024

Conversation

nick1udwig
Copy link
Contributor

Problem

Untitled

Solution

Only update the prompting_message when new one could conceivably be useful.

Testing

Ask me

Docs Update

TODO

Notes

This is a small change but should be carefully considered because

  1. It is technically breaking,
  2. Even if we decide we want it, we may want to do a similar thing with the Response None context case, in the match case just below.

Let's discuss @dr-frmr

@nick1udwig nick1udwig requested a review from dr-frmr June 18, 2024 04:59
@nick1udwig
Copy link
Contributor Author

nick1udwig commented Jun 18, 2024

Hmm, no I think this change as-is might be wrong. The problem is that the state can become fractured, e.g.:

self.last_blob = km.lazy_load_blob;
km.lazy_load_blob = None;
if expects_response.is_some() || km.rsvp.is_some() {
// update prompting_message iff there is someone to reply to
self.prompting_message = Some(km.clone());
}

You can end up in a situation where the prompting_message remains unchanged but the blob gets updated. This is strictly wrong behavior.

@nick1udwig
Copy link
Contributor Author

Well, I'm not certain that these two fields need to track 1-1, but it might lead to hard to debug situations. Need to think.

The way that this could be OK is that usually you only care about one thing at a given time: either you want to blob or you want to Respond. Sometimes you want both. In each of these cases you are OK, as written, since the blob is there when you expect it to be and the Response is routed to the most recent place you could possibly Respond to.

@nick1udwig
Copy link
Contributor Author

nick1udwig commented Jun 18, 2024

I think the main question becomes:

Is this mental model too hard to work with?

Existing mental model: I can get blob of & reply to most recent message (even if that message does not expect response/have RSVP).

New mental model: I can get blob of most recent message. I can reply to most recent message that expects response/has RSVP.

In first case you only need to know whether most recent message expects response/has RSVP.

In the case in this PR you need to know expects response/has RSVP for potentially many messages -- i.e. if you have N-1 most recent messages with not expects response/RSVP and then the Nth does er/RSVP, then the Nth is the one that will receive the response.

In one sense this PR is just enforcing expects response/RSVP to actually mean something from the side of the sender: you cannot reply to a message that does not ER/have RSVP.

@nick1udwig
Copy link
Contributor Author

Aha, this is just a resurfacing of an old idea #219 (comment)

@nick1udwig
Copy link
Contributor Author

Yeah, so that comment on #219 is when I was thinking about Kinode extensions. You can see the extension PR is right around the same time as that comment. And indeed, this change means that you can receive WS pushes without messing up your Req/Res context (since they are always not ER and no RSVP). This is really good! Also solves the similar #303.

HOWEVER: is it too much to ask of devs to understand the ER/RSVP of the Requests that come in?

Effectively this introduces a new type of Request to our system, the "one-off" Request. It does not start a new chain. It is just a packet of information disconnected from any other Req/Res chain and not interrupting current chains.

@nick1udwig
Copy link
Contributor Author

nick1udwig commented Jun 18, 2024

shapes at 24-06-18 10 25 26

alpha & beta here work on master. gamma & delta only work with this PR

@dr-frmr dr-frmr merged commit c709f6e into develop Jun 20, 2024
@dr-frmr dr-frmr deleted the hf/dont-overwrite-prompting-message-with-null branch June 20, 2024 10:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants