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.

Sign up for GitHub

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

Jump to bottom

Change Zinc seaside adaptors to allow ZnZincSeasideAdaptor to also stream #1319

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Change Zinc seaside adaptors to allow ZnZincSeasideAdaptor to also stream #1319

wants to merge 2 commits into from

Conversation

pdebruic
Copy link
Contributor

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 #flush is 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 ZnZincSeasideAdaptor can both stream and send normal responses and will stream responses where the request has a '_f' query field (added with the WAAnchorTag>>#safeToStream msg).

And then we drop the ZnZincStreamingServerAdaptor class.

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.

@pdebruic
change zinc streaming handling to use one server adaptor, and look fo…
48263f5 
…r the '_f' queryfield to use a chunked streaming response vs a normal response
@pdebruic
Copy link
Contributor Author

The other thing I'd like to do is change the #initialRequest: resposne to streaming by default. The http/1.1 chunked encoding translation to HTTP/2 & HTTP/3 can be handled by nginx or other servers. I think. On nginx in the location where you proxy to seaside add proxy_buffering off

@pdebruic
Copy link
Contributor Author

Oh and with these changes streamed responses in GemStone now work.

@pdebruic pdebruic mentioned this pull request Jun 25, 2022
Merged
@marschall
Copy link
Contributor

marschall commented Aug 21, 2022 •
edited

General thoughts on streaming:

  • Streaming is "problematic" because it makes error handling impossible. We have to decide on an HTTP status code before generating the response. When an exception happens when generating the response we can no longer change the status code, render an error page or redirect to an error page. In theory this could be solved with HTTP trailers. Unfortunately browser support seem lacking. Even if support was better we could probably not clear the HTTP body and I'm not sure a Location header to redirect to an error page would be allowed. See:
  • Streaming has no value for action continuations as the body is empty.
  • Streaming has limited value for render continuations. I can imagine it providing value in the following cases:
    • You want to flush the HTML header before starting rendering the body allowing the browser to prefetch the resources. This is implemented by WAFlushingRenderPhaseContinuation.
    • You're rendering a large page and want to make some initial data visible early to the client. Maybe you would be better off with lazy loading or infinite scrolling in JavaScript in this case.
  • Streaming provides the most value when you dynamically generate a large resource / document and don't want to load it into memory entirely.

Given these constraints I still believe the current approach with WAComboResponse and flushing on demand in the very specific cases where we want it is the most practical approach.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
Platform-GemStone Platform-Pharo Zinc
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@pdebruic @marschall