Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

DataReader gives empty result. #18

Open
tranquvis opened this issue Jul 4, 2017 · 6 comments
Open

DataReader gives empty result. #18

tranquvis opened this issue Jul 4, 2017 · 6 comments

Comments

@tranquvis
Copy link

I run a http server for my ui test. In order to check the app's request, I defined a route and tried to read the request body with DataReader as described.

Everything works fine except reading the body. Most time the body is empty.
The following should demonstrate the problem:

mockServer.router["/test"] = JSONResponse() { requestInfo -> Any in
  let bodyStream = requestInfo["swsgi.input"] as! SWSGIInput
  DataReader.read(bodyStream) { data in
    let text = String(data: data, encoding: .utf8)
    print(text)
  }
  print("return response")
  return []
}
        
var request = URLRequest(url: URL(string: "http://localhost:8080/test")!, cachePolicy: NSURLRequest.CachePolicy.reloadIgnoringLocalCacheData, timeoutInterval: 10)
request.httpMethod = "POST"
request.httpBody = "test value".data(using: .utf8)
URLSession(configuration: URLSessionConfiguration.default).dataTask(with: request).resume()
print("request sent")

Most time the read text is empty and I get the following output:

request sent
return response
Optional("")

Sometimes the text is correct and I get the following output:

request sent
Optional("test value")
return response

As you can see, the data is read after the response was returned here.

Please help me to solve this problem. Without this problem the library would be great.

I think it's also interesting that it works this way, without a router:

server = DefaultHTTPServer(eventLoop: eventLoop, port: 8080) {
    (
    environ: [String: Any],
    startResponse: ((String, [(String, String)]) -> Void),
    sendBody: ((Data) -> Void)
    ) in
    // Start HTTP response
    let pathInfo = environ["PATH_INFO"]! as! String
    let bodyStream = environ["swsgi.input"] as! SWSGIInput
    DataReader.read(bodyStream) { data in
        let text = String(data: data, encoding: .utf8)
        print(text)
    }
    startResponse("200 OK", [])
}
tranquvis added a commit to troii/Embassy that referenced this issue Jul 7, 2017
Connection is left open until the request has been completely read.
@tranquvis
Copy link
Author

I found the reason of this bug. The problem is located in Embassy.
Embassy aborts the connection when the response finishes and does not take care if the request was completely read. Embassy only waits for the header.
This is the same issue as #5.

Solutions:

One but in my opinion not a good possibility is to add a sleep as suggested here. Then the request body, which was read in 1 second is initially received with the header. However, when you receive a big request, the time does not suffice in order to read the whole body. Then you will only get the first part.

A better solution is to leave the connection open until the whole request has been read.
I implemented this behaviour in this fork: https://github.com/troii/Embassy

@fangpenlin
Copy link
Contributor

@tranquvis I think this is expected behavior

Please reference to https://github.com/envoy/Embassy#sendbody

As you see, we end the responding by sendBody with empty data, in your example without router, you never call sendBody, not even with data, of course the response never ends (or maybe it gets timeout)

And for JSONResponse, it basically wraps around DataResponse.

And the DataResponse, get the data returned from the function you passed in, call sendBody immediately if it's not empty then send a following empty Data to end the response

            if !data.isEmpty {
                sendBody(data)
            }
            sendBody(Data())

In this way, of course the response is ended immediately.

The constructor you are using is meant for returning simple data without any async operation, that's why it won't wait for reading POST data body or any other HTTP body data. To solve the problem, you should use another constructor that passes async send body method into the handler.

This one

    public init(
        statusCode: Int = 200,
        statusMessage: String = "OK",
        contentType: String = "application/json",
        jsonWritingOptions: JSONSerialization.WritingOptions = .prettyPrinted,
        headers: [(String, String)] = [],
        handler: @escaping (_ environ: [String: Any], _ sendJSON: @escaping (Any) -> Void) -> Void
    ) {

So instead, you should write something like this

mockServer.router["/test"] = JSONResponse() { (requestInfo, sendJSON) -> Any in
  let bodyStream = requestInfo["swsgi.input"] as! SWSGIInput
  DataReader.read(bodyStream) { data in
    let text = String(data: data, encoding: .utf8)
    print(text)
    sendJSON([])
  }
  print("return response")
}

as you call sendJSON after you are done with the data reading, it will only end the response after finishing reading body.

@tranquvis
Copy link
Author

Hi @fangpenlin,

I thought that this is expected behaviour.
Although it is a good performance feature for the server, I think the async behaviour is rarely needed in ui tests.

It would be great if you could provide an explanation in your readme and an additional WebApp for request reading.

Maybe you like my implementation:

import Embassy
import EnvoyAmbassador

struct RequestEvaluator: WebApp {
    public static var active: RequestEvaluator?
    
    public let requestHandler: (Data) -> WebApp
    private let semaphore = DispatchSemaphore(value: 0)
    
    public init(_ requestHandler: @escaping (Data) -> WebApp) {
        self.requestHandler = requestHandler
        RequestEvaluator.active = self
    }
    
    public init<T>(jsonHandler: @escaping (T?) -> WebApp) {
        self.init() { data in
            let jsonObject = try? JSONSerialization.jsonObject(
                with: data,
                options: .allowFragments
            )
            return jsonHandler(jsonObject as? T)
        }
    }
    
    public func app(
        _ environ: [String: Any],
        startResponse: @escaping ((String, [(String, String)]) -> Void),
        sendBody: @escaping ((Data) -> Void)
        ) {
        let input = environ["swsgi.input"] as! SWSGIInput
        var isBodyReceived = false
        DataReader.read(input) { data in
            guard !isBodyReceived else { return }
            isBodyReceived = true
            
            let nextApp = self.requestHandler(data)
            nextApp.app(environ, startResponse: startResponse, sendBody: sendBody)
            self.semaphore.signal()
        }
    }
    
    public func wait(timeout: TimeInterval = 10) -> Bool {
        return semaphore.wait(timeout: .now() + timeout) == .success
    }
}

Usage example

  • Listen on a http request and make assertions on it.
  • Redirect the Request to a JSONResponse WebAppand respond with an empty dictionary.
    (You can also redirect to any other WebApp.)
  • Assert that the request is received during 2 seconds and wait for it.
mockServer.router["/addTask"] = RequestEvaluator(jsonHandler: { (requestDict: NSDictionary!) in
    XCTAssert(requestDict["name"] as! String == "test")
    return JSONResponse() { _ in return [:] }
})

app.tap() // Interact with the app and trigger request.
XCTAssert(RequestEvaluator.active!.wait(timeout: 2))

RequestEvaluator also provides a initializer for handling raw data.

Problem with DataReader.

Maybe you have noticed the following section in my RequestEvaluator.

var isBodyReceived = false
DataReader.read(input) { data in
    guard !isBodyReceived else { return }
    isBodyReceived = true

This check is necessary because the data handler is called twice when starting the response inside the handler.
This is caused by the HTTPConnection class in Embassy.
inputHandler is set to nil after the handler is called and the handler triggers handleConnectionClosed, which calls the handler too.

Maybe you want to move inputHandler = nil above handler(Data()).

private func handleBodyData(_ data: Data) {
    guard let handler = inputHandler else {
        fatalError("Not suppose to read body data when input handler is not provided")
    }
    handler(data)
    readDataLength += data.count
    // we finish reading all the content, send EOF to input handler
    if let length = contentLength, readDataLength >= length {
        handler(Data())
        inputHandler = nil
    }
}

// called to handle connection closed
private func handleConnectionClosed(_ reason: Transport.CloseReason) {
    logger.info("Connection closed, reason=\(reason)")
    close()
    if let handler = inputHandler {
        handler(Data())
        inputHandler = nil
    }
    if let callback = closedCallback {
        callback()
    }
}

@ahti
Copy link

ahti commented Apr 17, 2018

I'm experiencing the same issue as @tranquvis. A minified version of my handler is this:

JSONResponse { env, respond in
    let input = env["swsgi.input"] as! SWSGIInput
    JSONReader.read(input) { data in
        // do something with data
        respond(someValueDepeingOnData)
    }
}

The block passed to JSONReader.read is called within the call to respond and thus executed twice.

I can also confirm that moving the inputHandler = nil rows above the handler(...) calls like @tranquvis suggested resolves the issue for me, so if that does not break anything else I'd love to see that done in a new release.

@fangpenlin
Copy link
Contributor

@tranquvis @ahti thanks for the feedback. For waiting async operation in the testing, we usually don't use DispatchSemaphore, we use XCTestExpectation instead

ref: https://nshipster.com/xctestcase/

So in our real-life UI test case, we will do things like this

let postEntryExp = expectation(description: "entry posted")
router[DefaultRouter.postEntriesPath] = DelayResponse(JSONResponse(handler: ({
    environ, sendJSON in
    XCTAssertEqual(environ["REQUEST_METHOD"] as? String, "POST")
    XCTAssertEqual(environ["HTTP_AUTHORIZATION"] as? String, "Bearer MOCK_TOKEN")
    JSONReader.read(environ["swsgi.input"] as! SWSGIInput) { json in
        let json = json as? [String: Any]
        let data = json?["data"] as? [String: Any]
        let attrs = data?["attributes"] as? [String: Any]
        XCTAssertEqual(attrs?["full-name"] as? String, fullName)

        sendJSON(["data": ["attributes": ["approval-status": ["status": "blocked"]]]])
        postEntryExp.fulfill()
    }
})))
waitForExpectations(timeout: 15, handler: nil)

As this is pretty much the recommended way of doing async stuff with iOS UI test, I think it's better to keep it the same way.

And for async, it's actually pretty common in our use case, we may like to delay, make it timeout, or many other thing we could do with async operations.

But however, yes, the API design is a little bit confusing. Maybe I shouldn't make response class to provide both async and sync operation in the same time. I think it's better to make them two different classes with obvious name what they are doing instead. I saw many issues opened not because of bug in the code, it's more misunderstanding about the async nature of Embassy and sometime mix with sync stuff. So I will take this into account, we may change the API in the future in the newer version to provide a more clear class design which is easier to understand. Also maybe trying to make common use case less confusing and error-prone.

@sushant-here
Copy link

For those that are facing the issue with the input callback being called twice. We are facing the same issue - however we fixed it by wrapping our DataResponse in a DelayResponse

ie....

DataResponse(
    statusCode: 200,
    statusMessage: "success",
    contentType: "application/xml"
) { ... }

becomes.....

DelayResponse(DataResponse(
     statusCode: 200,
     statusMessage: "success",
     contentType: "application/xml"
) { ... }, delay: DelayResponse.Delay.delay(seconds: 1))

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants