POC: Store read_state in process dictionary

[alisinabh]

Hi @mtrudel,

This can be a potential temporary for issues like #260 and #294. It uses process dictionary to store the read_state in the HTTP1 adapter. For now it handles the error and relies on the state in dictionary (which probably is not a good idea). We can also fail, close the connection and log a better message helping users with debugging their codebases.

I personally don't like using process dictionary but in this case it seemed to me like a good use-case (at least for short while to help with integration of bandit).

P.S This is just a POC and if you think it can be useful we can work on actually implementing it properly :)

[mtrudel]

Ah! The process dictionary is a great idea for this (maybe also self() messaging, as Plug does with @plug_sent?)

But, as it happens (and I've done a bad job of communicating this; that's on me), I'm midway through refactoring basically the entirety of both HTTP stacks to split out the transport concerns for each protocol from the shared HTTP semantic concerns (which will be shared between them once I'm done). The first step is up at #286 and describes part of the long-term plan I've got in place there. The HTTP/1 stack is up on the bench in pieces as we speak, and I hope to have it up for review in the next week or so (phoenixframework/phoenix#5706 distracted me a bit this week otherwise it would already be up).

Once that lands, I think this is a great idea. I don't know if we need to keep any substantial part of state in there though; all we need is enough to know if there's been a dropped conn somewhere. Maybe if we track the number of mutations of conn that we expect both inside the conn itself, and also in the process dict, and then raise an error if they mismatch when you attempt to make a conn call (or when the request ends)? Like:

def call(conn, _opts) do
  # pdict and conn both contain `num_calls: 0`
  {:ok, body, _conn} = read_req_body(conn)
  $ pdcit contains `num_calls: 1`, conn still contains `num_calls: 0` (since we didn't rebind the newly returned one
  
  # This call will explode with a `raise "Looks like you didn't track your conns properly"` error
  put_resp(conn, 200, "OK")
end

We could even make this an opt-in thing via configuration to help folks track these errors down. Then our first line of defence on issues like #294 would be to enable the track_calls option which would enable the above.

In any case though, I think this makes the most sense to wait for until the shared semantics work lands, since we'll get it on two stacks for the price of one at that point.

WDYT?

[alisinabh]

I'm pretty much aligned with all the points you raised here. And I actually love the idea to keep the number of mutations in pdict instead of a specific part of the state.

and I've done a bad job of communicating this; that's on me

I don't believe so. It was well communicated to me at least :)

Anyways, my intention for this PR was not get get merged but just wanted to share a potential way to detect these cases as it is very confusing. Maybe you can make a new issue for this or track this in your personal todo and close this PR. (feel free to close)

Thank you for the great work. I was really happy to see bandit as THE DEFAULT choice for new phoenix projects. 🚀

[ryanwinchester]

Smart 🤔

[mtrudel]

Closing for hygiene; @alisinabh this is still on my radar!