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

Avoid flushing for each SseEventBuilder entry #30912

Closed
ratacolita opened this issue Jul 18, 2023 · 7 comments
Closed

Avoid flushing for each SseEventBuilder entry #30912

ratacolita opened this issue Jul 18, 2023 · 7 comments
Assignees
Labels
in: web Issues in web modules (web, webmvc, webflux, websocket) type: enhancement A general enhancement
Milestone

Comments

@ratacolita
Copy link

Affects: 5.x/6.x

I'm working with Spring MVC, especially with Server-Sent Events (SSE). Now, it is always flushing data every time SSE event is sent. This is good for fast updates, but it can have bad effects on performance if many updates are being sent. It can create much network traffic and use more server CPU.

So, I suggest an improvement. I want to add 'lazy flushing' option. This gives choice to users when they want the flush operation to happen. With lazy flushing, system can choose best time to flush data, which could use resources better and also group updates together.

In our use case we could handle twice the number of clients receiving updates using about 1/3 of the CPU.

// org.springframework.web.servlet.mvc.method.annotation.ResponseBodyEmitterReturnValueHandler

		private <T> void sendInternal(T data, @Nullable MediaType mediaType) throws IOException {
			for (HttpMessageConverter<?> converter : ResponseBodyEmitterReturnValueHandler.this.sseMessageConverters) {
				if (converter.canWrite(data.getClass(), mediaType)) {
					((HttpMessageConverter<T>) converter).write(data, mediaType, this.outputMessage);
                                        // here we want to have some control over flush or not
					if(!this.isLazyFlush) {
						this.outputMessage.flush();
					}
					return;
				}
			}
			throw new IllegalArgumentException("No suitable converter for " + data.getClass());
		}
		
// org.springframework.web.servlet.mvc.method.annotation.SseEmitter

	public void send(SseEventBuilder builder) throws IOException {
		Iterable<DataWithMediaType> dataToSend = builder.build();
		synchronized (this) {
			for (DataWithMediaType entry : dataToSend) {
				super.send(entry.getData(), entry.getMediaType());
			}
		}
		// flush if necessary at the end of the send operation (maintain the old behavior)
		if(this.isLazyFlush()) {
			flush();
		}
	}

This is a scatch of that we did.

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged or decided on label Jul 18, 2023
@bclozel
Copy link
Member

bclozel commented Jul 19, 2023

Thanks for the insights @ratacolita

Can you elaborate on how and from where this "lazy flush" flag is being controlled? Is it on the emitter itself? How are you deciding when to turn it off and on?

@bclozel bclozel added status: waiting-for-feedback We need additional information before we can continue in: web Issues in web modules (web, webmvc, webflux, websocket) type: enhancement A general enhancement labels Jul 19, 2023
@ratacolita
Copy link
Author

Yes. We've created a method "isLazyFlush" on the ResponseBodyEmitter switch always returns false (because I didn't know the other use cases for this class) and overloaded the same method on the SseEmitter class. The SseEmitter itself got a new constructor with the option of setting its flag. So, once it set for the emitter its done.

@spring-projects-issues spring-projects-issues added status: feedback-provided Feedback has been provided and removed status: waiting-for-feedback We need additional information before we can continue labels Jul 19, 2023
@bclozel
Copy link
Member

bclozel commented Jul 19, 2023

I see, so this setting is completely disabling flushing for the entire lifetime of the emitter, until the very end. If that's the case, I don't think we can add such a feature here since this goes against the core use case for SSE. Maybe in your case SSE is not the right call and @Async should be used instead?

I'm closing this issue for now, but we can reopen if it turns out there is more to this. Thanks!

@bclozel bclozel closed this as not planned Won't fix, can't repro, duplicate, stale Jul 19, 2023
@bclozel bclozel added status: declined A suggestion or change that we don't feel we should currently apply and removed status: waiting-for-triage An issue we've not yet triaged or decided on status: feedback-provided Feedback has been provided labels Jul 19, 2023
@ratacolita
Copy link
Author

Regarding your suggestion to use @async, while it's a great feature for handling data production, it doesn't necessarily address the issue at hand. Even with asynchronous data production, we could still face network and CPU overhead if each small update is sent as a separate packet.

I believe this feature request warrants further consideration. It could potentially lead to significant performance improvements in certain scenarios without affecting the core use case of SSE.

You see. If we are using SseEmitter#send(SseEmitter.SseEventBuilder), we already have produced the data we are going to delivery inside the event builder. What the benefit of flushing the data in each item of the event build instead of flushing at once in the end of writing?
On the TCP level, flushing at each piece of data will produce a new tcp packet. If each event if every small, say 100 bytes, 10 events will produce 10 packets instead of one packet if we were flushing in batches. That will almost always be a overhead with no practical benefit.

@bclozel
Copy link
Member

bclozel commented Jul 19, 2023

Your comment is making a case against SSE itself. SSE is about sending data to clients as soon as it's available. If you'd like to buffer data upstream and only send when a few messages are available, it's really up to you. Delaying the send operation until the end completely breaks the use case. Again, using application/json is a better choice in this case.

@ratacolita
Copy link
Author

ratacolita commented Jul 19, 2023

SseEventBuilder does exactly this, it buffers the the 'lines' of events.
Its build method doc says: "Return one or more Object-MediaType pairs to write via".
If we have many pairs, we have a good oportunity of sending all at once, don't we?
We could also extend the builder to take a adittional parameter which would tell how to flush the items, wouldn't be great?
That would require much less refactoring of existing code and will allow for greater flexibility of this aspect.

@bclozel
Copy link
Member

bclozel commented Jul 19, 2023

Starting from an actual use case with SseEventBuilder and not a potential solution, I can see now where we could improve things. Let me have a look.

@bclozel bclozel reopened this Jul 19, 2023
@bclozel bclozel added status: waiting-for-triage An issue we've not yet triaged or decided on and removed status: declined A suggestion or change that we don't feel we should currently apply labels Jul 19, 2023
@bclozel bclozel self-assigned this Jul 19, 2023
@bclozel bclozel changed the title Improve: Add Option for Lazy Flushing in Server-Sent Events (2x perf improvement) Avoid flushing for each SseEventBuilder entry Jul 19, 2023
@bclozel bclozel removed the status: waiting-for-triage An issue we've not yet triaged or decided on label Aug 4, 2023
@bclozel bclozel modified the milestones: 6.1.0-M4, 6.0.12 Aug 4, 2023
@bclozel bclozel closed this as completed in e83793b Aug 4, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
in: web Issues in web modules (web, webmvc, webflux, websocket) type: enhancement A general enhancement
Projects
None yet
Development

No branches or pull requests

3 participants