View Issue Details

IDProjectCategoryView StatusLast Update
0003039GNUnetGNSpublic2019-02-28 11:17
ReporterChristian GrothoffAssigned Toschanzen 
PrioritynoneSeverityfeatureReproducibilityN/A
Status closedResolutionfixed 
Product VersionSVN HEAD 
Target Version0.11.0Fixed in Version0.11.0 
Summary0003039: gnunet-gns-proxy does not support persistent connections
DescriptionEspecially for SSL connections, supporting this would improve performance.

Implementing this will be a bit tricky:
1) when can we clean up the socks5request -- no longer on a mere
   MHD request completed callback; we need to distinguish between
   request completion and connection completion (and MHD does not
   expose the latter right now)
2) we need to set the content-length header properly in the MHD
   response; but right now we may not have that header when
   we create the response object, and MHD does not allow us to
   set it later
TagsNo tags attached.

Activities

schanzen

2015-07-10 18:13

developer   ~0009410

Last edited: 2015-07-10 18:18

View 2 revisions

This is very likely also an issue for pre-flight CORS requests.
I observed that usually a CORS response from the server has the "Connection: keep-alive" header set, while the proxy has "Connection: close,close". All other headers are the same. The browser will fail to accept the following request in the latter case.

This makes One-page JS applications that call REST APIs unusable.

I would consider this to be more that just a feature.

Christian Grothoff

2015-07-10 18:24

manager   ~0009411

I should note that with semi-recent MHD changes, this *can* now be fixed. But not before 0.10.2 and other more critical issues I need to look at first (of course, if you want to do it Martin, feel free ;-)).

schanzen

2015-07-10 21:36

developer   ~0009414

Yes if you point me to the MHD stuff I need then I can do this no problem. I kind of need it myself. Mail is ok as well.

Christian Grothoff

2015-07-11 00:16

manager   ~0009416

MHD_OPTION_NOTIFY_CONNECTION should be what you need. Using that callback, you can associate the socks5 request with the MHD connection as long as it lasts, and tear it down only when the browser actually closes the connection.

At least in theory ;-). How quickly do you need it? If necessary, I might be willing to try hard to find the time to help.

schanzen

2015-07-11 13:44

developer   ~0009417

I can put it on my todo list. I found a workaround for my needs atm using 2 firefox plugins. Eventually I want to tackle this though.
After a quick inspection I figured it would take me >1 day.

Whenever you have time it would be nice if you could help!

schanzen

2016-08-29 12:01

developer   ~0011075

Last edited: 2016-08-29 13:34

View 3 revisions

r37823 contains an initial implementation.

Three open questions:

1. If Content-length is set, the standard says that the connection should be closed anyway (by the client): https://www.w3.org/Protocols/rfc2616/rfc2616-sec8.html. So is this relevant to persistent connections??
It might even be sensible for the proxy to always speak HTTP 1.1 (with the client). Then we could just always strip the Content-Length header and be done with it.

2. Currently (at times), we set both Transfer-encoding: chunked as well as Content-length (from the original server). This seems to be wrong/inconsistent. Not sure how to handle this.

3. Right not I am filtering Expect: 100-continue request headers as this results in unnecessarily complicated response handling. I don't know if that is reasonable but the current solution makes CORS pre-flights such as http://arunranga.com/examples/access-control/preflightInvocation.html work.

- Martin

schanzen

2016-08-29 19:31

developer   ~0011077

Last edited: 2016-08-29 19:32

View 3 revisions

r37843 has bug fixes and major performance boost.

Current minor bugs that should maybe be put into new reports:

- Content-Length is stripped from responses as this clashed with microhttpd's chunked encoding
- No pipelining. However, if you look @Wikipedia most browsers to not support/use this anyway and for a variety of reasons I do not believe it is actually such a great performance boost.

@Christian: I think we can close this after you review it.

Christian Grothoff

2016-08-30 13:18

manager   ~0011079

Ok, one of the things I saw was that you're looking up some connection state by MHD_CONNECTION_INFO_FD (in O(n)), while the idea was to store the per-socket/connection state in MHD_CONNECTION_INFO_SOCKET_CONTEXT (in O(1)). So that should be changed.

1/2: This stripping of the content-length header or forcing chunked encoding makes no sense. I also don't read the standard as saying that the connection should be closed if content-length is set. In fact, if content-length is set, we can avoid the inefficient chunked encoding and still persist the TCP connection for DOWNLOADS. Or are you discussing the case of an *upload* (8.2.2?). Here, if the client attempted an upload and got an error code back (which may happen before the upload is complete, if it is an HTTP 1.0 connection without 100 continue), it must close. Still, we should keep the content-length header also for uploads (i.e. so that the server CAN check length requirements, also not all servers support chunked encoding for uploads). Basically, we should mess with as little as possible here.

3) I don't think filtering "Expect: 100-continue" is right either. We should pass them to the server, and forward the server's response as before. However, a potential issue here is that MHD usually automatically generates a "100 continue" reply when we first get a request and don't immediately generate a response. But, MHD_suspend_connection() may in this case offer a way around this. Not tested, though.

schanzen

2016-08-30 14:37

developer   ~0011080

Ok the problem is that currently MHD always seems to set transfer-encoding to chunked, I am guessing because of the way we create the response.
This in not compatible with a Content-length header though (not fatal, but gives you a warning in wireshark).

Continue: The problem is/was that even if the browser does not send an Expect: 100-continue header, cURL does.
Then, if we forward a Continue _Response_ to the browser, it gives a warning as this was not expected (also not fatal).

Christian Grothoff

2016-08-30 14:48

manager   ~0011081

Yes, you need to create the response with a known size (by parsing content-length from cURL). Then MHD should not force chunked encoding on us.

schanzen

2018-06-07 22:48

developer   ~0013018

Did you fix this issue as well with your last few commits?

Christian Grothoff

2018-06-07 23:00

manager   ~0013019

I am not even sure. I did see some code that used to inject "connection: close" commented out. I fixed some issue with us incorrectly setting chunked encoding. But: I also noticed that the proxy forced closing a connection that could presumably have stayed open, but didn't figure out why yet. So the real answer here is: I don't know!

schanzen

2018-06-23 17:14

developer   ~0013061

Tested. This is fixed at least since e2604f5e9c7b30f8f404cc8390230afb4d219be8

Issue History

Date Modified Username Field Change
2013-09-19 09:37 Christian Grothoff New Issue
2013-09-19 09:37 Christian Grothoff Status new => assigned
2013-09-19 09:37 Christian Grothoff Assigned To => Matthias Wachs
2013-09-19 09:37 Christian Grothoff Assigned To Matthias Wachs =>
2013-09-19 09:37 Christian Grothoff Status assigned => confirmed
2013-10-07 00:16 Christian Grothoff Priority normal => low
2013-10-20 20:37 Christian Grothoff Assigned To => Christian Grothoff
2013-10-20 20:37 Christian Grothoff Status confirmed => assigned
2013-10-27 16:25 Christian Grothoff Assigned To Christian Grothoff =>
2013-10-27 16:25 Christian Grothoff Priority low => none
2013-10-27 16:25 Christian Grothoff Status assigned => confirmed
2013-10-27 16:25 Christian Grothoff Target Version 0.11.0pre66 =>
2015-07-10 18:13 schanzen Note Added: 0009410
2015-07-10 18:18 schanzen Note Edited: 0009410 View Revisions
2015-07-10 18:24 Christian Grothoff Note Added: 0009411
2015-07-10 21:36 schanzen Note Added: 0009414
2015-07-10 21:59 schanzen Assigned To => schanzen
2015-07-10 21:59 schanzen Status confirmed => assigned
2015-07-10 22:00 schanzen Severity feature => minor
2015-07-10 22:00 schanzen Target Version => SVN HEAD
2015-07-10 22:01 schanzen Severity minor => major
2015-07-11 00:16 Christian Grothoff Note Added: 0009416
2015-07-11 13:44 schanzen Note Added: 0009417
2016-08-29 12:01 schanzen Note Added: 0011075
2016-08-29 12:02 schanzen Note Edited: 0011075 View Revisions
2016-08-29 13:34 schanzen Note Edited: 0011075 View Revisions
2016-08-29 19:31 schanzen Note Added: 0011077
2016-08-29 19:31 schanzen Note Edited: 0011077 View Revisions
2016-08-29 19:32 schanzen Note Edited: 0011077 View Revisions
2016-08-30 13:18 Christian Grothoff Note Added: 0011079
2016-08-30 13:19 Christian Grothoff Severity major => feature
2016-08-30 14:37 schanzen Note Added: 0011080
2016-08-30 14:48 Christian Grothoff Note Added: 0011081
2018-06-07 22:48 schanzen Note Added: 0013018
2018-06-07 23:00 Christian Grothoff Note Added: 0013019
2018-06-23 17:14 schanzen Status assigned => resolved
2018-06-23 17:14 schanzen Resolution open => fixed
2018-06-23 17:14 schanzen Note Added: 0013061
2019-02-12 09:20 Christian Grothoff Target Version SVN HEAD => 0.11.0
2019-02-20 12:24 Christian Grothoff Fixed in Version => 0.11.0
2019-02-28 11:17 Christian Grothoff Status resolved => closed