Skip to content

Conversation

@milt
Copy link
Member

@milt milt commented May 4, 2022

LRS-68 Currently the async multipart body build fn simply omits nil attachments. It should instead make the multipart result invalid, since the status cannot be changed. It is also currently utilized for sync responses, which is extra overhead.

Separating the sync and async approaches to building multipart responses (and testing them for parity) will let us have different behaviors WRT errors in mid-multipart on async.

After more research, the universal (sync + async) use of a channel body is probably more desirable as it allows even sync implementations to return things like File for attachment content which are passed for direct handling by pedestal.

This leaves the problem with the current impl that it simply omits attachments when errors occur, rather than terminating the response (and thus returning an invalid multipart), which is the only indication a client will get that the response is invalid.

This PR modifies the two async body formation functions (json and multipart) to accept a ::lrsp/async-error keyword in lieu of a statement, attachment or header. Upon receiving it they will immediately close the stream.

In practice this has no effect on sync impls like lrsql where an eager result is provided.

See the multipart body tests for examples of early termination: https://github.com/yetanalytics/lrs/blob/bd55295491e33523dd0c83c5c86cedf6389c3025/src/test/com/yetanalytics/lrs/pedestal/interceptor/xapi/statements/attachment/response_test.cljc

@milt milt changed the title LRS-68 Handle errors in async multipart LRS-68 Handle Multipart Errors May 5, 2022
@milt milt requested a review from kelvinqian00 May 5, 2022 20:54
@milt milt marked this pull request as ready for review May 5, 2022 20:54
@milt milt changed the title LRS-68 Handle Multipart Errors LRS-68 Handle Async Multipart Errors May 5, 2022
@kelvinqian00
Copy link
Contributor

Also don't forget to describe this revised behavior in the documentation. Probably advice best suited for the application level (since that's where the user docs are written), but since this will now apply to all apps that use this lrs lib it might be a good idea to add a section in the README for "things to note" like this.

@milt milt merged commit 48e1983 into master May 6, 2022
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

Successfully merging this pull request may close these issues.

3 participants