Change Zinc seaside adaptors to allow ZnZincSeasideAdaptor to also stream#1319
Open
pdebruic wants to merge 2 commits intoSeasideSt:masterfrom
Open
Change Zinc seaside adaptors to allow ZnZincSeasideAdaptor to also stream#1319pdebruic wants to merge 2 commits intoSeasideSt:masterfrom
pdebruic wants to merge 2 commits intoSeasideSt:masterfrom
Conversation
…r the '_f' queryfield to use a chunked streaming response vs a normal response
Contributor
Author
|
The other thing I'd like to do is change the |
Contributor
Author
|
Oh and with these changes streamed responses in GemStone now work. |
Contributor
|
General thoughts on streaming:
Given these constraints I still believe the current approach with |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
This is more of a "I think we should do this some way" rather than a "this is the way" PR.
The issue I'm trying to address is that some responses are OK to stream (e.g. navigational) and some responses are not OK to stream (e.g. those in transactions, where there might be transaction conflicts that happen while processing the request).
This is/can be a problem in GemStone which has automatic retries when there are transaction conflicts. Under a streaming regime the browser would get multiple attempts at loading a full page, e.g. so multiple
<head>sections & partial<body>tags, per request, one per retry.Given how the Zinc adaptor class hierarchy is set up it can also be tricky to make sure the transactional responses aren't streamed (e.g. by ensuring
#flushis never sent), especially when e.g. a database transaction fails.The bookkeeping of keeping track of what has been streamed to the client in error and what should be sent to correct that so they see the correct result of their click seems like nightmare fuel.
So I'm proposing to change it to where the
ZnZincSeasideAdaptorcan both stream and send normal responses and will stream responses where the request has a'_f'query field (added with theWAAnchorTag>>#safeToStreammsg).And then we drop the
ZnZincStreamingServerAdaptorclass.I've got changes for GemStone too but need to/will figure out how add them to this PR.
In GemStone the streamed responses currently do not work (every request gets a new session!) because the "meat" of the request/response processing happens outside of the retry-on-transaction-conflict-commit-on-success apparatus. While streaming the request/response processing happens while writing the response on the stream.