Skip to content

GH-8678: Make Buffer Overflow Strategy Configurable in IntegrationWebSocketContainer #8679

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

Closed
wants to merge 2 commits into from
Closed

GH-8678: Make Buffer Overflow Strategy Configurable in IntegrationWebSocketContainer #8679

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
0d8e89c
GH-8678: Make buffer overflow strategy configurable in IntegrationWeb…
ColoredCarrot Jul 16, 2023
5dd271a
GH-8678: Add test, remove explicit default
ColoredCarrot Jul 17, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
29 changes: 28 additions & 1 deletion ...rc/main/java/org/springframework/integration/websocket/IntegrationWebSocketContainer.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@
import org.apache.commons.logging.LogFactory;

import org.springframework.beans.factory.DisposableBean;
import org.springframework.lang.NonNull;
import org.springframework.util.Assert;
import org.springframework.util.ReflectionUtils;
import org.springframework.web.socket.CloseStatus;
Expand Down Expand Up @@ -67,6 +68,9 @@ public abstract class IntegrationWebSocketContainer implements DisposableBean {

public static final int DEFAULT_SEND_BUFFER_SIZE = 512 * 1024;

public static final ConcurrentWebSocketSessionDecorator.OverflowStrategy DEFAULT_SEND_BUFFER_OVERFLOW_STRATEGY =
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think we need this extra property for a default value: we simply can initialize our sendBufferOverflowStrategy with that ConcurrentWebSocketSessionDecorator.OverflowStrategy.TERMINATE as default value.
Or better to have this as null and chose an appropriate ctor when we instantiate ConcurrentWebSocketSessionDecorator

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I considered the latter approach as well. I thought that having the default be explicit in this class, in addition to ConcurrentWebSocketSessionDecorator, would relieve users of having to open up ConcurrentWebSocketSessionDecorator to see the default.
We could mention that TERMINATE is the default in the Javadoc, but I feel like that would more than likely not get updated if the default is ever changed in ConcurrentWebSocketSessionDecorator. Thoughts?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, to have a single place of truth, I'd prefer a @see ConcurrentWebSocketSessionDecorator in our new setter for this strategy.
Right, user would need to look there for more info, but then we would not not to worry about breaking change ever.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fair enough, I'll get that implemented :)

ConcurrentWebSocketSessionDecorator.OverflowStrategy.TERMINATE;

protected final Log logger = LogFactory.getLog(getClass()); // NOSONAR

protected final Lock lock = new ReentrantLock();
Expand All @@ -83,6 +87,10 @@ public abstract class IntegrationWebSocketContainer implements DisposableBean {

private int sendBufferSizeLimit = DEFAULT_SEND_BUFFER_SIZE;

@NonNull
private ConcurrentWebSocketSessionDecorator.OverflowStrategy sendBufferOverflowStrategy =
DEFAULT_SEND_BUFFER_OVERFLOW_STRATEGY;

public void setSendTimeLimit(int sendTimeLimit) {
this.sendTimeLimit = sendTimeLimit;
}
Expand All @@ -91,6 +99,24 @@ public void setSendBufferSizeLimit(int sendBufferSizeLimit) {
this.sendBufferSizeLimit = sendBufferSizeLimit;
}

/**
* Set the send buffer overflow strategy.
* <p>
* Concurrently generated outbound messages are buffered if sending is slow.
* This strategy determines the behavior when the buffer has reached the limit
* configured with {@link #setSendBufferSizeLimit}.
* <p>
* By default, the session associated with the culpable message is terminated.
*
* @param overflowStrategy The overflow strategy to use (see {@link ConcurrentWebSocketSessionDecorator.OverflowStrategy}).
*
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Our preferences do not have blank lines in method Javadocs: https://github.com/spring-projects/spring-framework/wiki/Code-Style#javadoc-formatting

* @see ConcurrentWebSocketSessionDecorator
*/
public void setSendBufferOverflowStrategy(@NonNull ConcurrentWebSocketSessionDecorator.OverflowStrategy overflowStrategy) {
Assert.notNull(overflowStrategy, "Overflow strategy must not be null");
this.sendBufferOverflowStrategy = overflowStrategy;
}

public void setMessageListener(WebSocketListener messageListener) {
Assert.state(this.messageListener == null || this.messageListener.equals(messageListener),
"'messageListener' is already configured");
Expand Down Expand Up @@ -190,7 +216,8 @@ public void afterConnectionEstablished(WebSocketSession sessionToDecorate)
WebSocketSession session =
new ConcurrentWebSocketSessionDecorator(sessionToDecorate,
IntegrationWebSocketContainer.this.sendTimeLimit,
IntegrationWebSocketContainer.this.sendBufferSizeLimit);
IntegrationWebSocketContainer.this.sendBufferSizeLimit,
IntegrationWebSocketContainer.this.sendBufferOverflowStrategy);

IntegrationWebSocketContainer.this.sessions.put(session.getId(), session);
if (IntegrationWebSocketContainer.this.logger.isDebugEnabled()) {
Expand Down