Set HTTP::Headers in read-only mode after HTTP::Headers were written #8877

asterite · 2020-03-04T12:58:48Z

Sija · 2020-03-04T13:11:10Z

src/http/headers.cr

+  def read_only?
+    @read_only
  end


Suggested change

def read_only?

@read_only

end

getter? read_only : Bool

j8r · 2020-03-04T13:07:27Z

src/exception.cr

+class ReadOnlyError < Exception
+  def initialize(message)
+    super(message)
+  end


Why defining an initialize here?

Because the message is optional in Exception. I want to force users of this exception to provide why the type was set to read-only.

@asterite If so, then it needs to be defined with String argument restriction, otherwise you could pass a nil as well.

This error is supposed to be re-used elsewhere? It may be a better idea to have a dedicated error for reading a read-only header.

Having both will be the best, to rescue only headers errors, or only read-only errors - or both.

Sure here, why using a global ReadOnly error then?
I don't think all ReadOnly exceptions (if any) would be on this case.

Just create a dedicated error where it is used; in the HTTP::Headers module - more importantly if not planned to be reused elsewhere.

Because the standard library shouldn't be raising Exception, it should be raising specific errors. Slice was already doing that and I thought about using the same exception type because the reason is the same: an object was set to read-only mode.

Sure, just wondering about where to place this very error, to provide more context, which will prevent to specify HTTP::Headers in the message (and just say headers)...
That's not a big deal then.

I was thinking of something like Cannot modify headers (HTTP::Headers::ReadOnlyError) instead of HTTP::Headers are in read-only (ReadOnlyError).

Just my opinion, but I don't think discussing these things matter.

bcardiff · 2020-03-04T15:52:37Z

I think the top-level error is fine, and eventually a freezable module could become handy.

Do you recall why you found freeze confusing? Although since we are not implementing it in Object, I am good with going on read_only route.

asterite · 2020-03-04T15:54:50Z

Maybe freeze is not confusing but I only found that term in Ruby. Also, Slice already uses read_only so I thought reusing that name for consistency was good.

vlazar · 2020-03-04T17:21:30Z

Not requesting a change, just want to point out that freeze name is more common nowadays :)
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Object/freeze

bcardiff · 2020-03-04T17:24:55Z

@vlazar I would reserve freeze only for a more general way to freeze objects, enforced by the compiler/runtime implicitly. I am not saying that that would/could happen. But since that is the usage in other languages, I would prevent the overlap and probably generate some confusion.

z64 · 2020-03-04T18:49:01Z

I'm open to suggestions about other ways to implement this.

To be honest, the original issue to me sounds like something the library should have solved by providing an immutable view, and not expose HTTP::Headers directly in the first place.. If we want to provide this in the standard library though, I'd like to explore generating a compile-time error with the current Crystal language:

Provide Headers.build that yields a builder object, returning a finalized HTTP::Headers
Provide Headers.new(hash) or other ctors for compatible header layouts
Make Headers immutable

We would still need some way for HTTP clients to supplement header "ergonomics", for example, supplying the correct Content-Length header or other default header values that the user didn't supply.

In which case, I'd probably suggest a "lower level" HTTP::Headers::Writer for serializing Headers or header values to sockets / IO that HTTP::Client would use after serializing the user's headers. This should hopefully mitigate the cost of otherwise providing an awkward #merge method that returns an update copy, but maybe that's not a big deal performance-wise.. I don't think header collections get terribly large in most apps? I don't know, honestly. Maybe users would still like to use it, as that semantic is pretty well understood.

Then, #8712 should be a compile-time error for frameworks that directly pass the user HTTP::Headers instances.

This has other analogs in our std, for concepts that construct Strings, but of course these don't have the same consequences as Headers:

HTTP::Params.build
JSON.build
String.build

Downsides would be that the deprecation period for the mutable methods would affect a lot of code, I imagine, so I don't think the above is compelling enough. But, WDYT?

src/http/headers.cr

straight-shoota · 2020-03-04T20:21:47Z

@z64 The problem with a builder approach is that HTTP headers are often not constructed at a single place, but dispersed over several handlers, middleware, framework code etc. I don't see how a builder could improve that without removing the ability to set header values somewhere in the HTTP handling stack (as long as it's before sending the first byte).

z64 · 2020-03-04T21:07:58Z

Right. Conceptually Headers::Writer would be the mutable counterpart, which I leave open-ended in this proposal.. it should be whatever it needs to be to allow what you described. Perhaps it's just exactly what HTTP::Headers is right now (but perhaps Headers::Writable would be a better name then, I don't know).

The intention was to divide Headers into mutable / immutable counterparts, so that it should be obvious to whoever is handling the type what the intention is, backed up by the compiler.

Headers.build would be for simple use cases (passing to one-shot HTTP::Client.get, for example), and streamed across a Writer internally.

asterite · 2020-03-04T21:21:08Z

I understand that that would in theory bring a bit more type-safety.

In practice, as soon as you make this mistake you'll find out while testing the app. I believe doing it the functional approach is more suitable to functional languages, not Crystal. It would make the code a lot harder to understand and follow, for a very small benefit.

straight-shoota · 2020-03-04T21:54:05Z

@z64 Yes, Headers currently works like a builder but it skips a dedicated immutable representation and directly serializes to an IO. It also doubles as a regular reader, which may make it a bit more complex. But I agree with @asterite that the current API probably offers the best usability.
Having a dedicated builder wouldn't help with issues that arise from the order in which the headers are modified and serialised.

RX14 · 2020-03-11T00:44:37Z

src/http/headers.cr

@@ -356,4 +377,10 @@ struct HTTP::Headers
      end
    end
  end
+
+  private def check_writeable


It's writable not writeable.

asterite · 2020-04-08T12:12:47Z

Ready for another review

spec/std/http/headers_spec.cr

asterite · 2020-04-09T11:11:15Z

Ready for yet another review. I had to add a way to unmark it as sent, because the request processor reuses http headers. So, I made it a property.

straight-shoota · 2020-04-09T11:21:41Z

spec/std/http/headers_spec.cr

+      headers["Foo"].should eq("Bar")
+    end
+
+    it "can still read them" do


This spec seems duplicate.

straight-shoota · 2020-04-09T11:23:08Z

src/http/server/response.cr

@@ -50,6 +50,7 @@ class HTTP::Server
    def reset
      # This method is called by RequestProcessor to avoid allocating a new instance for each iteration.
      @headers.clear
+      @headers.sent = false


Shouldn't this happen in Headers#clear?

That was my first thought, but I'm not sure. It's not obvious that any time you want to clear the headers you also want to mark them as not sent.

I think that clear method delegates to Hash, so effectively jumping over the sent? guard. I think HTTP::Headers should stop forwarding stuff to Hash. Then we need to move sent = false above this line.

This looks like it will take a long time to settle, so for now I'm abandoning this PR. Anyone feel free to continue this.

clear already changes the headers, so it can't work any other way than with sent == false. It either needs to call check_not_sent or implicitly set sent = false. When doing the latter, we can also revert from sent= to mark_as_send! and the only way to remove the sent flag is calling clear. That should actually cover all use cases.
Maybe clear shoud be renamed to reset to better communicate its purpose.

The thing is, imagine the headers were already sent. Someone calls clear, appends a few headers, everything works fine. Except that it has no effect on the output because the headers were sent and you didn't get a runtime error.

This is what I'm trying to avoid.

Then clear shouldn't be part of the public API or make it clear!/reset! to emphasize the possible dangers related to it.

asterite · 2020-04-19T21:48:03Z

Closing. Let's discuss what API we want in the issue and then (someone) send a single PR without discussions.

asterite changed the title ~~Read only~~ #8712 Mar 4, 2020

asterite changed the title ~~#8712~~ Set HTTP::Headers in read-only mode after HTTP::Headers were written Mar 4, 2020

Sija reviewed Mar 4, 2020

View reviewed changes

j8r reviewed Mar 4, 2020

View reviewed changes

straight-shoota reviewed Mar 4, 2020

View reviewed changes

src/http/headers.cr Outdated Show resolved Hide resolved

RX14 reviewed Mar 11, 2020

View reviewed changes

asterite added 4 commits April 8, 2020 09:10

Introduce ReadOnlyError and let slice raise it if read-only

799453d

Set HTTP::Headers in read-only mode after HTTP::Headers were written

3035cd9

Prevent removing read-only from HTTP::Headers

b6bb79c

HTTP::Headers: rename read_only to mark_as_sent

0b07017

asterite force-pushed the read-only branch from f62ff6d to 0b07017 Compare April 8, 2020 12:12

straight-shoota reviewed Apr 8, 2020

View reviewed changes

spec/std/http/headers_spec.cr Outdated Show resolved Hide resolved

RX14 approved these changes Apr 8, 2020

View reviewed changes

Rename mark_as_sent! to sent, and clear it after a request

d3d46a3

straight-shoota reviewed Apr 9, 2020

View reviewed changes

asterite closed this Apr 19, 2020

asterite deleted the read-only branch April 19, 2020 21:48

wonderix mentioned this pull request Jan 2, 2021

Read-Only functionality for HTTP::Headers #10176

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Set HTTP::Headers in read-only mode after HTTP::Headers were written #8877

Set HTTP::Headers in read-only mode after HTTP::Headers were written #8877

asterite commented Mar 4, 2020 •

edited

Loading

Sija Mar 4, 2020

j8r Mar 4, 2020

asterite Mar 4, 2020

Sija Mar 4, 2020 •

edited

Loading

j8r Mar 4, 2020

j8r Mar 4, 2020

j8r Mar 4, 2020

asterite Mar 4, 2020

j8r Mar 4, 2020

j8r Mar 4, 2020

asterite Mar 4, 2020

bcardiff commented Mar 4, 2020

asterite commented Mar 4, 2020

vlazar commented Mar 4, 2020

bcardiff commented Mar 4, 2020

z64 commented Mar 4, 2020 •

edited

Loading

straight-shoota commented Mar 4, 2020

z64 commented Mar 4, 2020

asterite commented Mar 4, 2020

straight-shoota commented Mar 4, 2020

RX14 Mar 11, 2020

straight-shoota Apr 8, 2020

asterite commented Apr 8, 2020

asterite commented Apr 9, 2020

straight-shoota Apr 9, 2020

straight-shoota Apr 9, 2020

asterite Apr 9, 2020

straight-shoota Apr 9, 2020 •

edited

Loading

asterite Apr 9, 2020

straight-shoota Apr 9, 2020

asterite commented Apr 19, 2020

Set HTTP::Headers in read-only mode after HTTP::Headers were written #8877

Set HTTP::Headers in read-only mode after HTTP::Headers were written #8877

Conversation

asterite commented Mar 4, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Sija Mar 4, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bcardiff commented Mar 4, 2020

asterite commented Mar 4, 2020

vlazar commented Mar 4, 2020

bcardiff commented Mar 4, 2020

z64 commented Mar 4, 2020 • edited Loading

straight-shoota commented Mar 4, 2020

z64 commented Mar 4, 2020

asterite commented Mar 4, 2020

straight-shoota commented Mar 4, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

asterite commented Apr 8, 2020

asterite commented Apr 9, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

straight-shoota Apr 9, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

asterite commented Apr 19, 2020

asterite commented Mar 4, 2020 •

edited

Loading

Sija Mar 4, 2020 •

edited

Loading

z64 commented Mar 4, 2020 •

edited

Loading

straight-shoota Apr 9, 2020 •

edited

Loading