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

Remove blank line after subscription headers #85

Open
canadaduane opened this issue Feb 9, 2021 · 16 comments
Open

Remove blank line after subscription headers #85

canadaduane opened this issue Feb 9, 2021 · 16 comments

Comments

@canadaduane
Copy link
Member

When a client makes a GET request with Subscribe: header set, the server responds without version, parents, and merge-type headers in the canonical response headers. Instead, they are sent in a subsequent block that is only accessible with special braid-header ("virtual header") parsing.

https://github.com/braid-work/braid-spec/blob/bf179d8c84ea48b5cb1ea4faf23c6ea9bfc28416/draft-toomim-httpbis-braid-http-03.txt#L406-L472

What if we combine the first version's headers with the canonical HTTP response headers? See PR #84 for concrete change.

This would have the following desirable properties:

  1. A non-Braid-aware fetch could still fetch a Braid resource and parse Version, Parents, and Merge-Type headers.
  2. A regular GET and a Subscribe-GET could have the same headers and (initial) body. This simplifies implementation details (e.g. the "if subscribe do braid http / else do regular http" block could become "do braid http; continue sending versions if subscribe").
@toomim
Copy link
Member

toomim commented Feb 10, 2021

I think this is a very interesting proposal! Good idea, Duane.

I especially appreciate point (2). I am not yet sure that point (1) will work, because we are returning a different status code and aren't closing the connection after sending the body, but that's an empirical question that I'd love to learn the answer to.

I want to think on this for a while and then give my thoughts, but I'm quite intrigued.

@toomim
Copy link
Member

toomim commented Feb 11, 2021

I just realized that this conflicts with the specification for the "all-caught-up" version: https://github.com/braid-work/braid-spec/pull/79/files

         HTTP/1.1 209 Subscription
          Subscribe: keep-alive
          Version: "ej4lhb9z78"                       <-- Current Version

          Version: "ej4lhb9z78"                       + Stream of updates
          Parents: "oakwn5b8qh", "uc9zwhw7mf"         |
          Content-Type: application/json              |
          Merge-Type: sync9                           |
          Content-Length: 64                          |
                                                      |
          [{"text": "Hi, everyone!",                  |
            "author": {"link": "/user/tommy"}}]       V
                                                      <-- Now caught up

That first Version: "ej4lhb9z78" needs to be separated by a blank line in order to be meaningful here. If we remove the blank line, we should probably rename the first Version to Initial-Version or something.

This also brings up another issue to consider— it's common for a server to send not the current version to a new connecting client, but rather a whole set of versions. This is important, for instance, when other clients might be actively editing the resource, and might create a patch based on an old version that the new connecting client needs to know about.

The unfortunate consequence of this is that it becomes less conceptually elegant to group the "first version" in with the initial response. If a client that doesn't know about Braid Subscriptions connects, and just looks at the initial version that comes across the wire, it'll actually be looking at an old out-of-date version. The current version will often come later in the stream.

@toomim toomim added this to the Braid-HTTP 03 milestone Feb 13, 2021
@canadaduane
Copy link
Member Author

canadaduane commented Feb 16, 2021

That first Version: "ej4lhb9z78" needs to be separated by a blank line in order to be meaningful here. If we remove the blank line, we should probably rename the first Version to Initial-Version or something.

Does the addition of the Current-Versions header resolve the above concern?

182e8a6

@toomim
Copy link
Member

toomim commented Feb 22, 2021

Yes, it does resolve it for me! Although I do wonder if Current-Versions is a name that we want to stick with? I don't recall us discussing that name fully.

@canadaduane
Copy link
Member Author

I like Current-Versions but if we need to discuss maybe we can open another issue.

@toomim
Copy link
Member

toomim commented Feb 22, 2021

Notes from https://braid.org/meeting-3:

Cases that interact with this proposal:

  • First message is document snapshot
  • Client receives all operations
    • In the first message
    • In the first N messages
  • Client reconnects and wants changes since last time

Question:

  • Is it possible for non-braid HTTP clients to fall back to interpreting subscription requests as a single version?
    • The subscription response 209 might break this

Use-case:

  • Could you have a poor-man's subscribe by polling?
    • Mike: I can think of how to do this, and would like to write up an explanation. This doesn't need the subscribe header.
    • Seph: We could also do a hybrid approach by saying "Subscribe, but limit me to N messages/bytes", and then the client can poll opening a new connection for a new amount.

@josephg
Copy link
Contributor

josephg commented Mar 22, 2021

Punt to 04

@josephg josephg removed this from the Braid-HTTP 03 milestone Mar 22, 2021
@toomim toomim changed the title GET response's first version should be part of top-level HTTP headers & body Remove blank line after subscription headers Aug 20, 2023
@toomim
Copy link
Member

toomim commented Aug 20, 2023

(Removing Chair hat:) After some thought, I think this is a very good improvement. I'm on board!

This proposal will clarify that there are only TWO levels of data in Braid-HTTP subscriptions:

  1. Updates
  2. Patches

This will make things much more elegant. Notice that this will make GET and GET Subscribe responses look identical through the first update, aside from the Subscribe: header and status code.

Normal GET:

      Request:

         GET /hello

      Response:

         HTTP/1.1 200 OK
         Content-Type: application/json                     | Update
         Content-Length: 12                                 |
                                                            |
         Hello world!                                       | | Snapshot

GET + Subscribe:

      Request:

         GET /hello
         Subscribe: true

      Response:

         HTTP/1.1 209 Subscription
         Subscribe: true
         Content-Length: 12                                 | Update
                                                            |
         Hello world!                                       | | Snapshot

         Content-Length: 14                                 | Update
                                                            |
         Goodbye world!                                     | | Snapshot

I think the symmetry we're creating there is a very good thing.

The blank line in Subscriptions has been giving the impression that there are three levels of headers (e.g. see #96 (comment)):

  1. Top-level resource headers
  2. Update-level headers, for each new version
  3. Patch-level headers

However, I claim that there's no need for a distinction between 2 and 3. This false distinction has been confusing people. I think we should remove it.

Where this blank line came from

I think the blank line was probably introduced into our code accidentally while implementing, because when we use existing HTTP libraries, we have to generate and parse headers within the "body" of a message from the library's perspective, and the library automatically inserts a blank line between its headers and body. When we generated version: headers inside the body, we neglected to notice that the library inserted a blank line in between those headers and its own headers.

We'll need to modify our implementations so that they generate the headers for the first update into the existing HTTP header libraries, and then generate subsequent headers into the body of the message. And when reading messages, our parsers will need to copy the initial update's headers from the library's headers list, and then parse the headers for subsequent updates itself.

@toomim
Copy link
Member

toomim commented Aug 20, 2023

Concluding the other issues brought up in Meeting-3

These issues are orthogonal:

Cases that interact with this proposal:

  • First message is document snapshot

^ This is already an allowed server behavior, but we'll want a way for clients to request it when initiating a Subscription.

  • Client receives all operations
    • In the first message
    • In the first N messages

^ Denoting the initial set of operations is addressed by the "all-caught-up" Current-Version: header. #79

  • Client reconnects and wants changes since last time

^ This is handled with the Parents: header.

The following are suggestions for parameters that we'll want to add to the Subscribe header.

Question:

  • Is it possible for non-braid HTTP clients to fall back to interpreting subscription requests as a single version?
    • The subscription response 209 might break this
      Use-case:
  • Could you have a poor-man's subscribe by polling?
    • Mike: I can think of how to do this, and would like to write up an explanation. This doesn't need the subscribe header.
    • Seph: We could also do a hybrid approach by saying "Subscribe, but limit me to N messages/bytes", and then the client can poll opening a new connection for a new amount.

We have had a number of ideas for parameters to the Subscribe header bubble up, such as: #101, #89, #92, #80, #62, #54, #105, #115. We should probably consolidate these into a single issue, and write up a PR. Also, we should look at @brynbellomy's Redwood here, because he implemented such a set of parameters in Subscribe:.

@toomim
Copy link
Member

toomim commented Aug 21, 2023

I'm re-nominating this for consideration in draft 03, because:

  • the change to the spec should be very simple to author (it's just removing a blank line)
  • it makes the protocol much simpler to read: it's clearly 2 levels of updates, not 3

@toomim
Copy link
Member

toomim commented Aug 22, 2023

Notes from Meeting-67:

  • Rahul: the blank line could provide semantics of "this is the initial state" vs. "this is an update"

@toomim
Copy link
Member

toomim commented Aug 22, 2023

I've drafted this up in PR #121

@toomim toomim removed this from the Braid-HTTP 03 milestone Aug 22, 2023
@CxRes
Copy link

CxRes commented Aug 23, 2023

To expand on my suggestion:

  • A blank line after headers can be used to indicate to the client that there is no initial representation in this response and only updates will be sent.
  • You can think of the blank line as placeholder/proxy for the initial representation.
  • If an initial representation is sent, there are no blank lines. This will be consistent with normal GET.

@toomim
Copy link
Member

toomim commented Aug 23, 2023

there is no initial representation in this response and only updates will be sent

@CxRes are you describing the situation described in #105? What is a scenario in which there would be no initial representation? The one that comes to mind for me is that you subscribe to a resource with a Parents: header specifying the current version; thus there would be no new updates to send until a new change occurs. Is that what you are thinking of?

@CxRes
Copy link

CxRes commented Aug 23, 2023

Something like that. Suppose a client has already obtained a cached copy. They could subscribe with a request where they explicitly ask for a representation to be sent only if the event-id/version has changed, otherwise only updates be sent.

I have explicitly covered this in PREP. See implementation guidance under section 7 for such a Request and section 9.2 for a corresponding response.

@toomim
Copy link
Member

toomim commented Sep 18, 2023

I implemented this, and found it breaks with chrome'sfetch(). Details here: #121 (comment)

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

No branches or pull requests

4 participants