Transfer-Encoding:chunked tests #16678
Conversation
lib/pure/asynchttpserver.nim
Outdated
| # Skip \r\n (chunk terminating bytes per spec) | ||
| when compileOption("assertions"): | ||
| let separator = await client.recv(2) | ||
| assert separator == "\r\n" |
There was a problem hiding this comment.
can this fail in practice (eg client that violates spec)? if so, it's an external condition (not internal logic), so raising exception should be used instead
silently skipping the last 2 bytes can be really bad if they're not \r\n.
I doubt checking this condition has any real performance impact compared to rest of surrounding code.
There was a problem hiding this comment.
Well, it is external input, so sure, it can trigger the assert, but it would be a malformed request according to (my understanding of) the spec.
If the bytes are never received, the await should never finish, so I don't think an exception would be thrown.
If the bytes are received, but are not "\r\n", they simply get discarded. I also don't think this is exception worthy, especially if exceptions are explicitly disabled.
I'm not sure (and have no strong feelings) about what the "right" thing to do if the last two bytes are not "\r\n", but I would err towards saying that it is a malformed request (maybe return 400?) and is not a server exception.
There was a problem hiding this comment.
what do other languages do?
There was a problem hiding this comment.
Here's what Python does: https://github.com/python/cpython/blob/63298930fb531ba2bb4f23bc3b915dbf1e17e9e1/Lib/http/client.py#L558
if not chunk_left: # Can be 0 or None
if chunk_left is not None:
# We are at the end of chunk, discard chunk end
self._safe_read(2) # toss the CRLF at the end of the chunkwhere self._safe_read is defined as
def _safe_read(self, amt):
"""Read the number of bytes requested.
This function should be used when <amt> bytes "should" be present for
reading. If the bytes are truly not available (due to EOF), then the
IncompleteRead exception can be used to detect the problem.
"""
data = self.fp.read(amt)
if len(data) < amt:
raise IncompleteRead(data, amt-len(data))
return dataThere was a problem hiding this comment.
ok we can deal with it in future PR's then, keep the code as is (with when compileOption("assertions"): ...
maybe with a comment:
# xxx figure out whether to raise, assert or doAssert
There was a problem hiding this comment.
Instead of adding a TODO just do it properly: raise a ValueError.
There was a problem hiding this comment.
I agree with ValueError and that was my initial wish too in #16678 (comment)
|
|
||
| let port = Port(64123) | ||
| let server = newAsyncHttpServer() | ||
| discard server.serve(port, handler) |
There was a problem hiding this comment.
@vabresto in the meantime, add a comment:
# xxx figure out whether there's a better way than discarding a Future hereso we can move forward with this PR
|
followup to my recommendation in #16678 (comment), this works and is much better (not flaky): diff --git a/lib/pure/asynchttpserver.nim b/lib/pure/asynchttpserver.nim
index ecbed8028..4d029c5c7 100644
--- a/lib/pure/asynchttpserver.nim
+++ b/lib/pure/asynchttpserver.nim
@@ -71,6 +71,9 @@ type
maxBody: int ## The maximum content-length that will be read for the body.
maxFDs: int
+proc getSocket*(a: AsyncHttpServer): AsyncSocket {.since: (1,5,1).} =
+ a.socket
+
proc newAsyncHttpServer*(reuseAddr = true, reusePort = false,
maxBody = 8388608): AsyncHttpServer =
## Creates a new ``AsyncHttpServer`` instance.
diff --git a/tests/stdlib/tasynchttpserver_transferencoding.nim b/tests/stdlib/tasynchttpserver_transferencoding.nim
index 3c508902f..c384eae07 100644
--- a/tests/stdlib/tasynchttpserver_transferencoding.nim
+++ b/tests/stdlib/tasynchttpserver_transferencoding.nim
@@ -1,6 +1,9 @@
import httpclient, asynchttpserver, asyncdispatch, asyncfutures
import net
+import std/asyncnet
+import std/[nativesockets]
+
const postBegin = """
POST / HTTP/1.1
Host: 127.0.0.1:64123
@@ -19,9 +22,11 @@ template genTest(input, expected) =
sanity = true
await request.respond(Http200, "Good")
- let port = Port(64123)
let server = newAsyncHttpServer()
- discard server.serve(port, handler)
+ discard server.serve(Port(0), handler)
+ let port = getLocalAddr(server.getSocket.getFd, AF_INET)[1]
+
let data = postBegin & input
var socket = newSocket()
socket.connect("127.0.0.1", port)adding This is a common use case, and not just for writing tests. |
|
I will take a look and implement it tonight, but right away I notice that you have an |
sorry, I just removed it (was just for debugging).
not needed here, but in other situations, you'd use discard """
output: '''
blah blah
'''
""" |
lib/pure/asynchttpserver.nim
Outdated
| let chunk = await client.recv(bytesToRead) | ||
| request.body.add(chunk) | ||
| # Skip \r\n (chunk terminating bytes per spec) | ||
| when compileOption("assertions"): |
There was a problem hiding this comment.
You don't need this, assert is already removed by the compiler when this option is not specified.
There was a problem hiding this comment.
I was told in a different comment (or maybe IRC) that doing something like this would save an extra string allocation. Additionally, if one compiles with asserts off, will
let separator = await client.recv(2)
assert separator == "\r\n"cause an Unused identifier warning?
That said, I don't mind removing it as it definitely looks cleaner without it.
lib/pure/asynchttpserver.nim
Outdated
| # Skip \r\n (chunk terminating bytes per spec) | ||
| when compileOption("assertions"): | ||
| let separator = await client.recv(2) | ||
| assert separator == "\r\n" |
There was a problem hiding this comment.
Instead of adding a TODO just do it properly: raise a ValueError.
| await request.respond(Http200, "Good") | ||
|
|
||
| let server = newAsyncHttpServer() | ||
| discard server.serve(Port(0), handler) |
There was a problem hiding this comment.
(previous comment got lost)
discarding a future seems buggy (eg refs #11912)
note that #16695 doesn't have this issue (see its code with await server.serve(Port(5555), cb)), can you make something like it work? if not, add a comment:
# xxx figure out whether there's a better way than discarding a Future here
There was a problem hiding this comment.
So, I spent some time and dug through some docs, and with a bit of intuition and a lot of luck, I was able to replace this by using callSoon. I have no clue if that solution is any better than using discard, but I couldn't replicate what #16695 was doing.
Please advise.
There was a problem hiding this comment.
EDIT: I don't know if callSoon is the right thing here; runSleepLoop seems more complicated than it should, there may be a better, but I don't know which, so please add a
# xxx check if there is a better way so we can move forward with this PR
There was a problem hiding this comment.
It's definitely not the right thing here. All you need to do is use asyncCheck on the code that calls asynchttpserver's serve. I think you've made the code harder to work with by creating that genTest template which is not necessary, turn it into an async proc and then waitFor that async proc.
|
For some reason I can't reply to #16678 (comment) but I'm confused: I'm not sure what TODO you're talking about, but more importantly, I don't believe this should raise a |
seems fine; if needed followup work can allow customizing this |
Yeah, you're right, for some reason I thought I was looking at httpclient code. Too much context switching :) |
|
for whoever's reading this, the only remaining point is #16678 (comment) |
|
Sadly, I don't know enough about Nim's async to resolve #16678 (comment) myself. I've tried the following: but both error out with I also don't feel good about using |
Ping @dom96, @timotheecour |
|
No, it doesn't unfortunately. I rebased locally and tried both of the snippets linked above and both still had However, the variant with Here's why I think that works:
There was a question earlier about why the server is able to process this request if it is sleeping when the request arrives; my explanation is that we do I've also tried this snippet https://pastebin.com/wJwXpME3 after rebasing, per #16678 (comment) but this just stops at the serve (ie. the server is serving, but none of the rest of the code gets executed). As far as my understanding goes, the code currently in this PR is the "best" variant. If there's anything else I should try, please let me know. |
|
We can always improve it further in follow-up PRs. |
|
For reference, I explained how to resolve this here: https://irclogs.nim-lang.org/13-01-2021.html#23:38:06 |
|
@dom96 Yes, I saw that (and thanks for your help in IRC, I wouldn't have been able to get this far without help from you and the community), but I wasn't able to get it working, it had the exact same errors listed above. If you've gotten it to compile and run without errors, can you please make a PR? I'm still very new to Nim so I'd love to see what it was that I'm missing. Plus, if what I've done here isn't best practices, it would be great to have a correct implementation for people to refer to in the future. |
|
@vabresto this test seems flaky, see timotheecour#544 not sure whether it relates to discarded future. |
|
@timotheecour A few notes based on briefly looking around:
Can you try to update your branch? I would've expected that a lot more PR runs would've failed if this was being flaky. Other suggestion would be to try getting rid of the template (just copy-paste the code) and see if that works better, but note that I've run into issues before where running the code inside the template just once would be fine, but twice (like in a copy-paste) would fail. Other question is how flaky is it? Have you run into this before? |
that's unrelated; the CI logs shown was for a PR against latest devel (2b5841c)
not relevant as mentioned earlier
haven't tried
the test may fail on some low percentage of cases, but it's still a bug (a low percentage could translate in a high percentage depending on use cases). Since this test is joined in testament, it can also be that this bug triggers or not depending on unrelated conditions; one such example was the bug fixed in this PR #14327 but in your case the bug is likely different.
this would just obfuscate / work around the underlying bug; we should expose and find the bug, not hide it ;-) One way would be to run the test N times in a test PR (and still making it run inside megatest which seems to cause the issue); once there's a reliable way to reproduce the bug, it's easier to identify / fix.
1st time I see it, it may not be very flaky but it's still a bug that can impact user code if they use the same pattern |
|
Using |
|
I don't know if it is or isn't hiding a crash, just that I ran it a bunch of times and I didn't have any issues - like I said above, I don't know enough about Nim (and especially Nim async). I just did my best, and am happy to get feedback. That said, I did test that the test works and is correct. I tried changing the text that gets matched, and if I do change it, the test fails. This suggests to me that it is in fact a useful test. And for what it's worth, I was testing on OSX 10.15.6, so I don't know if it's particularly flaky on OSX. |
* Add tests and fix extra newlines in body * Fixes per comments * Slight rephrase per comments * Improvements per comments * Add getSocket to reduce test flakiness per comment * Remove unused lines from header * Add doc comment to getSocket per comment * Apply witchcraft to replace `discard Future` * Return HTTP 400 on bad encoding in request * Fix runnable example for getSocket * Fix import to fix runnable examples * Even more imports for the example * Better self documenting runnable example * Add missing import * Import from module with correct signature * Resolve port type mismatch
Follow up of PR #16636
This PR makes an important fix of excluding the newline separators from the final body, as this didn't come up in my use case and thus testing.
Additionally, it includes 4 tests: