-
Notifications
You must be signed in to change notification settings - Fork 4
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
Mark the loadXML call as blocking #28
Conversation
collectBinary(msg).flatMap[DecodeFailure, Elem] { chunk => | ||
source.setByteStream(new ByteArrayInputStream(chunk.toArray)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could avoid this strictness by converting the fs2 stream to an InputStream. I think we'd spend CPU to save memory with that approach.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I assume this gets targeted to 0.23?
@@ -49,7 +52,8 @@ trait ElemInstances { | |||
* | |||
* @return an XML element | |||
*/ | |||
implicit def xml[F[_]](implicit F: Concurrent[F]): EntityDecoder[F, Elem] = { | |||
@deprecated("Blocks. Use xmlDecoder with an Async constraint.", "0.23.12") | |||
def xml[F[_]](implicit F: Concurrent[F]): EntityDecoder[F, Elem] = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we care enough we can do a runtime check for Async
to delegate to the new implementation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That technique makes me squeamish, but we've adopted it enough other places and I've never seen an accident scene from it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oops. I merged before resolving this. If we want, it could still be a separate PR.
I really hate introducing constraint churn, with what appears to be a better solution looming in 0.24 (I'm working on a bench). But what's here in 0.23 is just simply not safe. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Re-triggered CI on 0.23.
…-blocking Mark the loadXML call as blocking
Fixes #27. Source compatible for everyone who uses it implicitly and has an Async.
The need for Sync was offensive enough in JSON that we made a capability trait. Note that we can relax it back to
Concurrent
in #25.