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

Improvements for using xml-conduit for streaming XML protocols #85

Merged
merged 3 commits into from May 11, 2016

Conversation

Projects
None yet
2 participants
@abbradar
Copy link
Contributor

abbradar commented May 5, 2016

This consists of two parts:

  1. Add element{To,From}Events which allow one to render streams from elements rather than complete documents. For example, I use this to implement an XMPP client which first sends opening <stream> root element (manually encoded with [Event]) and then lots of messages (Elements) inside which are sent in real time via a conduit.
  2. Add renderBuilderFlush which allows one to control where downstream builderToByteStringFlush should flush message to the socket. This is required for streaming protocols where one is expected to send part of XML "document" (a "stanza" for XMPP) and then await for reply.
-- this function doesn't support prettifying. Also it won't do certain other
-- optimizations, like using self-closed tags.
renderBuilderFlush :: Monad m => RenderSettings -> Conduit (Flush Event) m (Flush Builder)
renderBuilderFlush settings@(RenderSettings { rsPretty = True }) = error "renderBuilderFlush: prettifying is not supported"

This comment has been minimized.

Copy link
@snoyberg

snoyberg May 10, 2016

Owner

Is there an inherent reason this isn't supported?

This comment has been minimized.

Copy link
@snoyberg

snoyberg May 10, 2016

Owner

Never mind, I read the comment. I see that there's a reason, I guess I'm just not following it. In any event, I think adding a comment explaining that rsPretty is ignored in this function would be preferable to errorring out.

This comment has been minimized.

Copy link
@abbradar

abbradar May 10, 2016

Author Contributor

Sorry, I'm bad at comments and explanations -_-

It was made this way to guarantee that Flush would emit all piped data further, acting as a barrier. Consider this scenario: I first send Chunk EventBeginDocument{}, then Chunk EventBeginElement{} and then Flush. At this point I expect that output would consist of prologue and an opening tag. But if something relies on peeking, it can't be done because we would need the next Chunk to get event out of it. Hence, disabled everything that relies on peeking. This actually happens in XMPP where I need to send an opening tag and then wait for the server to response before sending next messages.

On the other hand, while writing this I realized that it can actually be done if I would peek at the Flush stream directly and decide conservatively if Flush is encountered. For example, in the case of isClosed I can just default it to False when that happens. This would get me a nice optimization, too (less text to send for self-closing tags). Stay tuned for an updated version of the patch!

@abbradar abbradar force-pushed the abbradar:master branch from efe4c67 to ccc63ba May 10, 2016

@abbradar

This comment has been minimized.

Copy link
Contributor Author

abbradar commented May 10, 2016

I've pushed the second version of this patch -- now prettifying and emitting self-closed tags is supported. The code looks better IMO, too.

@abbradar abbradar force-pushed the abbradar:master branch from ccc63ba to 544075b May 10, 2016

@snoyberg

This comment has been minimized.

Copy link
Owner

snoyberg commented May 11, 2016

Looks good, thanks! Can you add some Haddocks to the top of all the newly exported functions?

@abbradar

This comment has been minimized.

Copy link
Contributor Author

abbradar commented May 11, 2016

Done for all *{From,To}Events -- are those descriptions OK?

@snoyberg snoyberg merged commit c024b13 into snoyberg:master May 11, 2016

@snoyberg

This comment has been minimized.

Copy link
Owner

snoyberg commented May 11, 2016

LGTM, thanks!

@abbradar

This comment has been minimized.

Copy link
Contributor Author

abbradar commented May 11, 2016

Thank you for review ^_^

snoyberg added a commit that referenced this pull request May 11, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.