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

Warnings or auto-draining about not touched HttpRequest dataBytes #183

Open
ktoso opened this Issue Sep 8, 2016 · 14 comments

Comments

Projects
None yet
2 participants
@ktoso
Copy link
Member

ktoso commented Sep 8, 2016

Issue by ktoso
Tuesday Oct 13, 2015 at 08:22 GMT
Originally opened as akka/akka#18716


In the bypass merge we can figure out that once the user has applied the Request => Response,
we require the dataBytes source to have been materialized.
If it's not, we log a warning, and we could drain the dataBytes.

Split when will need to emit a special source, that we can inspect and see if it's been materialized or not.

@ktoso ktoso added this to the http-backlog milestone Sep 8, 2016

@ktoso

This comment has been minimized.

Copy link
Member Author

ktoso commented Sep 8, 2016

Comment by drewhk
Tuesday Oct 13, 2015 at 08:23 GMT


warnings and auto-draining.

@ktoso

This comment has been minimized.

Copy link
Member Author

ktoso commented Sep 8, 2016

Comment by ktoso
Tuesday Oct 13, 2015 at 08:23 GMT


We're working on fusing and things around Http, and this idea came up.
We'll be able to warn or consume the dataBytes for the users if the user forgot to drain the data from the request.

"Single actor Http pipeline, here we come!"

// cc @sirthias @jrudolph

@ktoso

This comment has been minimized.

Copy link
Member Author

ktoso commented Sep 8, 2016

Comment by jrudolph
Tuesday Oct 13, 2015 at 09:08 GMT


You may want to look at existing tickets. There are some that suggest similar things but don't mention the technical approach.

"Single actor Http pipeline, here we come!"

So, concatAll will be fusable?

@ktoso

This comment has been minimized.

Copy link
Member Author

ktoso commented Sep 8, 2016

Comment by ktoso
Tuesday Oct 13, 2015 at 09:30 GMT


Both concat and splitWhen will be mergable (i.e. in general streams of streams will be)

@ktoso

This comment has been minimized.

Copy link
Member Author

ktoso commented Sep 8, 2016

Comment by drewhk
Tuesday Oct 13, 2015 at 09:34 GMT


this ticket actually has nothing to do with fusing though, it just happened that we discussed this during our fusion discussion.

@ktoso

This comment has been minimized.

Copy link
Member Author

ktoso commented Sep 8, 2016

Comment by sirthias
Tuesday Oct 13, 2015 at 12:45 GMT


Interesting.
Note that auto-draining might not always be what I want, though!
For example, if the request contains an entity with a content-length that it beyond the configured threshold I want to send an error response and NOT drain the incoming byte stream.
So, this auto-draining would definitely have to have a config switch (which could default to on).

@ktoso

This comment has been minimized.

Copy link
Member Author

ktoso commented Sep 8, 2016

Comment by drewhk
Tuesday Oct 13, 2015 at 13:21 GMT


yes, that makes sense. We also want to log a Warning though, so it is not as much on vs off, but instead *warn vs fail?

@ktoso

This comment has been minimized.

Copy link
Member Author

ktoso commented Sep 8, 2016

Comment by sirthias
Tuesday Oct 13, 2015 at 13:51 GMT


Wouldn't it be something like

auto-drain-request-entity = on-with-warning # or `off`

?

@ktoso

This comment has been minimized.

Copy link
Member Author

ktoso commented Sep 8, 2016

Comment by drewhk
Tuesday Oct 13, 2015 at 13:56 GMT


maybe that. I am not sure, I think we will see how it works in practice in the end anyway. This is just a high level idea so far.

@ktoso

This comment has been minimized.

Copy link
Member Author

ktoso commented Sep 8, 2016

Comment by ktoso
Wednesday Dec 16, 2015 at 10:56 GMT


For reference, bypassMerge is gone, but the idea of this ticket is still valid.

@ktoso

This comment has been minimized.

Copy link
Member Author

ktoso commented Sep 8, 2016

Comment by rkuhn
Monday Mar 21, 2016 at 13:46 GMT


In addition to the discussed possibilities it could make sense to add an option that auto-drains the request entity for successful “early” responses, but not for unsuccessful ones—in the latter case it would emit the error and close the connection (because continuing afterwards would not be legal for HTTP). This yields the following list of possibilities:

  • no detection (current behavior)
  • warning for unconsumed entities
  • auto-drain unless rejected, in which case early response and close (should be default)
  • auto-drain always
@ktoso

This comment has been minimized.

Copy link
Member Author

ktoso commented Sep 8, 2016

Comment by ktoso
Thursday May 19, 2016 at 00:10 GMT


Relates to akka/akka#19602

@ktoso

This comment has been minimized.

Copy link
Member Author

ktoso commented Sep 8, 2016

Comment by ktoso
Saturday Jul 30, 2016 at 17:50 GMT


ping @rkuhn, if you have any new ideas please share :-)
We have not been working on this so far btw.

@toebbel

This comment has been minimized.

Copy link

toebbel commented Dec 8, 2017

I am interested in seeing similar warnings for ResponseEntities. We had a case, where we removed some lines that would consume a http ResponseEntity from an upstream service, however, the code lines that create the request were left in place. Since a cached host http connection pool is used, the allocated connections were never free'ed since their responses were neither read nor discarded.

Coming from http4s and fs2.Task we did not expect that Http().singleRequest performs the side-effect of checking out a connection of the pool, before the response entity is touched.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.