Multiple fixes for http libraries #1832
Conversation
src/lib/http/http_server.pl
Outdated
| close(ResponseStream) | ||
| chars_utf8bytes(ResponseText, ResponseBytes), | ||
| catch( | ||
| call_cleanup(maplist(put_byte(ResponseStream), ResponseBytes),close(ResponseStream)), |
There was a problem hiding this comment.
This is very expensive, since it writes every byte individually. Is there a way to use format/3 or phrase_to_stream/2 for this? It seems that should work.
Note that chars_utf8bytes/2 is also very expensive, and forfeits the compact internal string representation!
There was a problem hiding this comment.
This was actually what I needed to change in order to not generate corrupted responses (as you might remember from IRC). However, I'm going to try to change the type of the stream as you told me
There was a problem hiding this comment.
Thank you a lot! There could very well be another, unrelated, mistake hidden in Scryer's stream or string handling that is now only overshadowed by this workaround. format/3 or phrase_to_stream/2 should work, and if not, then we should correct this! I think a good test case tries, over and over, to send and receive messages, for many hours.
|
Thank you a lot for working on this! I added a comment for Prolog code which seems not ideal. |
| catch( | ||
| call_cleanup(format(ResponseStream, "~s", [ResponseText]),close(ResponseStream)), | ||
| error(existence_error(stream, _), _), | ||
| true |
There was a problem hiding this comment.
Is there a reason to suppress this exception if it occurs?
It seems it could be interesting to know whether sending failed!
There was a problem hiding this comment.
This might happen if the HTTP request was canceled by the client. At that point, we can safely ignore the error, there's no point in sending it again since we lost the connection, and don't stop the server.
|
I made some changes to the build definitions to fix the issue that caused the last build to fail. You can trigger the build again if you push any commit to your branch. |
* use reqwest for http_open (still uses Hyper underneath) * use Hyper 1.0.0-rc3 for server * Modify all internal handling of server
|
@aarroyoc Thanks for these fixes, should I merge them now? |
|
Yes! I've been using it to develop my new blog and is very stable. I still want to add more things as I need them but I can open another PR. The only "ugly" thing is that it uses a non-stable version of Hyper (1.0-RC3) and uses reqwest which bundles another Hyper (0.14). So the final binary has two Hypers... In theory, when Hyper 1.0 goes stable, reqwest should upgrade too and the dependency will be shared but I can't give you an estimation of how much time it is until that. |
In general we try to use less parts of Tokio (which can be problematic right now and actually it introduced a complex data race) while preparing Scryer to use Hyper 1.0 once it reaches final state. We use reqwest instead of Hyper for the client because in Hyper 1.0 they removed the high-level Client module we were using, reqwest is from the same authors, higher level but for our use case, still fine.
Note that none of this changes the external API that http_open and http_server define.