View Issue Details
ID | Project | Category | View Status | Date Submitted | Last Update |
---|---|---|---|---|---|
0003039 | GNUnet | GNS | public | 2013-09-19 09:37 | 2019-02-28 11:17 |
Reporter | Christian Grothoff | Assigned To | schanzen | ||
Priority | none | Severity | feature | Reproducibility | N/A |
Status | closed | Resolution | fixed | ||
Product Version | Git master | ||||
Target Version | 0.11.0 | Fixed in Version | 0.11.0 | ||
Summary | 0003039: gnunet-gns-proxy does not support persistent connections | ||||
Description | Especially 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 | ||||
Tags | No tags attached. | ||||
|
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. |
|
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 ;-)). |
|
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. |
|
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. |
|
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! |
|
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 |
|
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. |
|
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. |
|
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). |
|
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. |
|
Did you fix this issue as well with your last few commits? |
|
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! |
|
Tested. This is fixed at least since e2604f5e9c7b30f8f404cc8390230afb4d219be8 |
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 | |
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 | => Git master |
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 | |
2016-08-29 13:34 | schanzen | Note Edited: 0011075 | |
2016-08-29 19:31 | schanzen | Note Added: 0011077 | |
2016-08-29 19:31 | schanzen | Note Edited: 0011077 | |
2016-08-29 19:32 | schanzen | Note Edited: 0011077 | |
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 | Git master => 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 |