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

Distinguish null result from error #118

Closed
wants to merge 1 commit into from
Closed

Conversation

dvdplm
Copy link
Contributor

@dvdplm dvdplm commented Jul 2, 2020

Closes #117

When a remote endpoint replies with e.g. {"jsonrpc":"2.0","result":null,"id":1} we get a None from next_repsponse(). This is not an error so the current implementation is a bit confusing.
common::Response (and Serde) doesn't parse null as a valid JSON document (see https://stackoverflow.com/questions/8526995/is-null-valid-json-4-bytes-nothing-else) so this PR replaces null with [] which does parse. Not elegant but perhaps better than the status quo?

When a remote endpoint replies with e.g. `{"jsonrpc":"2.0","result":null,"id":1}` we get a `None` from `next_repsponse()`. This is not an error so the current implementation is a bit confusing.
`common::Response` (and Serde) doesn't parse `null` as a valid JSON document (see https://stackoverflow.com/questions/8526995/is-null-valid-json-4-bytes-nothing-else) so this PR replaces `null` with `[]` which does parse. Not elegant but perhaps better than the status quo?
@dvdplm dvdplm requested review from tomaka and tomusdrw July 2, 2020 13:23
@tomusdrw
Copy link
Contributor

tomusdrw commented Jul 2, 2020

It doesn't feel right: None probably just means that the stream is over, I don't see how null get's converted to None in that stream.

@svyatonik can you take a look? You have much better understanding of the codebase than I do.

@svyatonik
Copy link
Contributor

I think Pierre has most of knowledge here :) But imo Tomek is right. My understanding is:StreamExt::next() returns None if Stream::poll_next() returns Ready(None). Usually it means that the stream is finished. But here we deal with FuturesUnordered, which may also return Ready(None) when it is empty (and it is empty initially, or when all futures have completed). We have had the similar problem in #107.

Afaiu, the most straightforward solution would be to replace FuturesUnordered with channel here - i.e. whenever response is received, background thread sends it to the channel. And the next_response is just waiting for the first response it sees. I may be wrong, or Pierre may suggest a better solution - just sharing my thoughts :)

@dvdplm dvdplm closed this Oct 15, 2020
@dvdplm dvdplm deleted the dp/fix/parse-empty-values branch October 15, 2020 08:56
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.

Q: meaning of self.responses.next() returning None?
3 participants