This repository has been archived by the owner on Apr 26, 2024. It is now read-only.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Iteratively encode JSON responses #8013
Iteratively encode JSON responses #8013
Changes from 4 commits
7418fa9
f141196
133fe03
17798e6
69c5475
4579dbb
5675445
23d3cca
cf426ee
21e1bfe
6e25056
5a7399b
d43d309
9d86790
26cd7be
6df06f3
7469b07
3a3d288
b9d09c0
c00905c
65b9e90
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
Is this true? We're' not awaiting here so the reactor should not be able to spin?
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 think it's cut-and-pasted from elsewhere; but I seem to remember that it is true that this can be re-entrant.
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.
This was originally cut-and-pasted from the
NoRangeStaticProducer
and then tweaked a bit to be more readable.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 added some logging and I actually wasn't able to cause this to be re-entrant?
Anyway the question of "how does the reactor spin?" is convoluted, but I think I found the answer:
registerProducer
on theRequest
actually registers it with theHTTPChannel
HTTPChannel
then converts theIPullProducer
to anIPushProducer
.startStreaming
which generates acooperator
_PullToPush._pull
) yields after each call toresumeProducing
to give up control to the reactor.The result is that in one reactor "tick" this should generate <~1 KB of data.
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.
Ok, right interesting. Since this function is not a coroutine I don't think this can yield back to the reactor half way through the function
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.
Right, it "yields" when the function runs off, NOT in the middle of the function.
Do you think any changes are needed here?
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 guess I still don't really see what this comment is trying to convey.
write
may or may not cause the reactor to wake up immediately, and I don't see why we are trying to highlight that.If we want to highlight that this function may be re-entrant, it'd be good to mention what the code is doing to make that safe (I guess something to do with the fact that the write is the last thing we do in the function, and its only during
write
that we can become re-entrant?)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 believe it is important that you check if
self._request
still exists since something could have calledstopProducing
.Maybe it isn't useful to clarify -- I can remove the part of the comment if you'd like (or maybe move it above the
self._request
check?)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.
Saying that at by the check I think would be best
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.
@erikjohnston I clarified a bunch of comments! Let me know if it is better now!