Longer POST/PUT body fails inconsistently (tests attached)
dmzza opened this issue · 4 comments
I've created a test package with nearly the same SiteRoute used in vapor-routing's tests.
I demonstrate several things in these tests that I've observed in real-world use.
- Posting a short comment like the one in your test works fine every time
- Posting a medium-length comment usually works, but it will fail a few times out of 100 repetitions
- Posting a long comment almost always fails in the first twenty or so repetitions, and then it will succeed.
- The issue can be demonstrated on a route that parses the Comment, but also on a route that just accepts raw data. So the issue isn't related to JSONDecoder.
- Vapor's built-in router doesn't have this issue
- XCTVapor can't be used to demonstrate this issue, so I test by running my example server and making URLRequests against the running example server.
I've tried many things, but I've had no luck fixing this issue myself. I thought it might have been related to this open issue: vapor/vapor#2682 but that issue appears to be caused by the use of data.getString(at: 0, length: data.capacity) in the example project attached to that issue.
There are a few important things you should know about running these tests
- You need to run the server in Xcode before you run the tests
- When you run the tests while the server is running, it will ask you if you want to replace the server. Click "Add" instead of replacing it.
- The issue can be demonstrated just by running the tests once, but it is even more interesting when you run the tests repeatedly. If you haven't done this before, see this screenshot for instructions:
@dmzza Thanks for the report and repro!
We think the behavior probably has to do with how we read a Vapor request into the URLRouting's request type:
I'm really not sure how the Vapor team expects us to read data from a request, but it's a bummer that it fails per the issue you link to. I did find that if we explicitly kick off a request to collect the request bytes, it does work:
public func respond(
to request: Request,
chainingTo next: AsyncResponder
) async throws -> Response {
+ _ = try await request.body.collect().get()
guard let requestData = URLRequestData(request: request)
else { return try await next.respond(to: request) }But I'm not sure this is the "right" or "best" way of doing this (and I'm not sure if it causes problems for requests that don't have bodies).
Do you have any other ideas/solutions we could try? Or maybe there's a way to ping the Vapor team for an update? Seems like a problem that can crop up quickly.
I assumed that line 21 was the source of the problem too, so I tried replacing that code with this.
- let body: [UInt8]?
- if var buffer = request.body.data,
- let bytes = buffer.readBytes(length: buffer.readableBytes)
- {
- body = bytes
- } else {
- body = nil
- }
+ var bodyData: Data?
+ if let buffer = request.body.data,
+ let data = buffer.getData(at: buffer.readerIndex,
+ length: buffer.readableBytes,
+ byteTransferStrategy: .noCopy) {
+ bodyData = data
+ } else {
+ bodyData = nil
+ }I essentially copied that from Swift NIO's code for decoding JSON from a ByteBuffer https://github.com/apple/swift-nio/blob/dfd4bdad89c7369ce60e66259df2fe836b21f56c/Sources/NIOFoundationCompat/Codable%2BByteBuffer.swift#L31
But that solution didn't appear to affect my test results.
I'll try testing your idea with other requests to see if it could be a workaround for now until we figure out the right way.
I just noticed your commit. Thanks! I'll take a look and verify that it solves it for me.