http
http copied to clipboard
swift-server
Deep recursion in completion callback
In theory with current implementation of HTTPStreamingParser it is possible to run out of the stack. This problem can occur if a long chain of callback receiving methods (i.e. writeBody) is called. To solve this completion callback should never be called before method returns.
Because of this problem it looks like two different APIs for sync/async should exist. Right now I see no solution to perform sync operations via async API.
Can you provide a testcase or some example code that shows the problem occurring?
It is obvious: https://github.com/swift-server/http/blob/develop/Sources/HTTP/HTTPStreamingParser.swift#L421
The worse aspect is that this calls the done-cb before the queued write has actually finished! This needs to be passed along the stream, that's the whole point.
Also the method looks inefficient. It calls withUnsafeBytes just to get the count (potentially resulting in a temporary object just being constructed for this).
And then it copies over the data to a Data object. I don't remember what we decided on here, IMO we should use DispatchData to pass buffers around as this supports iovecs (and AFAIK Data doesn't, right?).
@aleksey-mashanov presumably you're referring to the fact that its an @escaping closure that doesn't actually escape the function?
And yes - as @helje5 says, it looks like its not actually associated with the write (which is async...)
I see no reason to use DispatchData because to construct DispatchData from Swift native types a copy of their buffers have to be done. As I promised I have been worked on improving this part of API for a while and have a better solution which supports iovecs and sendfile. I will make a PR when I have enough time to prepare it (within a month I think).
The main point of this issue is to raise discussion about sync/async API and how to achieve both.
A DispatchData doesn't require copying but supports custom deallocators, that is a key part of its design. (I'm not denying that there might be a better alternative, but DispatchData is much better than Data for this specific purpose (which presumably is the reason why it still exists ;-) )).
P.S.: Sorry for hijacking this PR, that really wasn't appropriate. If this is about sync/async again - that topic was also discussed on the list already. Maybe it should a) state its purpose better and b) link to the mailing list archives for context.
I don't see how this conflicts w/ synchronous use, a synchronous call would directly invoke the CB (funny enough - it is precisely what the current implementation gets wrong :-) ). In fact the callback is of little use in a synchronous implementation because by definition that always returns immediately ...
A DispatchData doesn't require copying but supports custom deallocators, that is a key part of its design. (I'm not denying that there might be a better alternative, but DispatchData is much better than Data for this specific purpose (which presumably is the reason why it still exists ;-) )).
Prove this with a code snippet plz. Lets imagine what we have an array of strings and we want to write it to the body.
I don't see how this conflicts w/ synchronous use, a synchronous call would directly invoke the CB (funny enough - it is precisely what the current implementation gets wrong :-) ). In fact the callback is of little use in a synchronous implementation because by definition that always returns immediately ...
Yes. And this is the problem itself: deep recursion. writeBody then callback then writeBody then callback ... and then out of the stack.
Synchronous code must not use callbacks.
There should be two HTTPResponseWriter protocols: one sync and another async. Server conforming to HTTPResponseSyncWriter supports sync operations, conforming to HTTPResponseAsyncWriter supports async, conforming to both - supports both. I see no advantages to mix them in one trying to administratively prohibit using of callbacks in sync code and prevent using of sync applications with async servers and vice verse.
@aleksey-mashanov the concurrency model that exists for Swift is async based. What would be the driver for there to be support for both sync and async APIs?
Wrt DispatchData, init(bytesNoCopy:deallocator:) 🙄. (and again, my comment wasn't about DispatchData vs something entirely different, but about DispatchData vs Data).
Sync works just fine, I already did a proof of concept implementation for the 1st revision of the proposal (mod_swift), I don't think anything relevant has changed. W/ a sync implementation the write returns when it is done, there is simply no reason to use the callback to schedule subsequent writes. (only if you actually want the recursion 🤓)
The concurrency model that exists for Swift is async based
Hm? Today no concurrency model for Swift exists at all. Only last week or so a first proposal for a maybe-Swift-5-one has been published by Chris Lattner ;-)
What would be the driver for there to be support for both sync and async APIs?
Well, because there are going to by synchronous implementations :-) But the current proposal should be OK for that - as far as I can see. Maybe I should redo the mod_swift proof of concept.
@seabaylea Swift itself has no concurrency model. It can be used with sync and async both. Personally in general I prefer sync because it is simpler to maintain large codebase but I use async for tasks there it suits better for some reasons.
The model provided by Apple in their Foundation and Dispatch frameworks can suit well to client side developers who want to write a little server side code but not to server side developers who want to use Swift in the way they used to with other popular server side languages.
Wrt DispatchData, init(bytesNoCopy:deallocator:) 🙄.
Complete snippet plz. I still don't understand. To be clear, UnsafeRawBufferPointers taken from String and Array are valid only inside withUnsafeBytes.
I put comments from @helje5 into two other issues so we can track them explicitly. They're both valid points, so I explained why I did what I did, and I'm happy to discuss better implementations.
But I didn't want those topics to get lost in this issue.
