- 
                Notifications
    You must be signed in to change notification settings 
- Fork 1.1k
GH-10083: Apply Nullability to TCP/IP module #10306
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
Conversation
dd36f78    to
    c0937d2      
    Compare
  
    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.
The usual annoying questions ;-)
        
          
                ...src/main/java/org/springframework/integration/ip/config/TcpConnectionFactoryFactoryBean.java
          
            Show resolved
            Hide resolved
        
              
          
                ...g-integration-ip/src/main/java/org/springframework/integration/ip/tcp/TcpInboundGateway.java
          
            Show resolved
            Hide resolved
        
              
          
                ...-integration-ip/src/main/java/org/springframework/integration/ip/tcp/TcpOutboundGateway.java
          
            Show resolved
            Hide resolved
        
              
          
                ...c/main/java/org/springframework/integration/ip/tcp/connection/AbstractConnectionFactory.java
          
            Show resolved
            Hide resolved
        
              
          
                ...on-ip/src/main/java/org/springframework/integration/ip/udp/UnicastSendingMessageHandler.java
              
                Outdated
          
            Show resolved
            Hide resolved
        
      c0937d2    to
    c77b69c      
    Compare
  
    Related to: spring-projects#10083 * Add `@NullMarked` to every package * Assume `BeanFactory` and `ApplicationEventPublisher` are not null in TCP/IP components. Therefore, fix all the failing tests to ensure those properties and their propagation via `afterPropertiesSet()`
…fType` instead of `try..catch`
* Remove `SyslogdTests.java` which are not tests.
…is set in the `onInit()` * Improve `UnicastSendingMessageHandler.restartAckThread()` to reflect the proper state of the `UnicastSendingMessageHandler`
c77b69c    to
    66817b7      
    Compare
  
    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.
LGTM! Thank you!
Will merge after successful build!
| if (connection == null) { | ||
| publishNoConnectionEvent(message, connectionId); | ||
| logger.error(() -> "Connection not found when processing reply " + reply + " for " + message); | ||
| return false; | 
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.
Just curious: This method always returns false, but I didn't look into detail. If this is desired, I wondered if for the sake of readability, this one should be changed to void.
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.
Good catch, @mrpiggi !
The contract is this:
@FunctionalInterface
public interface TcpListener {
	/**
	 * Called by a TCPConnection when a new message arrives.
	 * @param message The message.
	 * @return true if the message was intercepted
	 */
	boolean onMessage(Message<?> message);
}
So, we need to revise and return true at some point.
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.
FYI: #10314
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.
FYI: #10315
| catch (Exception ex) { | ||
| logger.error(ex, "Exception accepting new connection(s)"); | ||
| private void keyAcceptable(final Selector selector, @Nullable ServerSocketChannel server, final long now) { | ||
| if (server != null) { | 
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.
Just nitpicking: I tend to prefer if (server == null) { return; } as this reduces the level of indentation
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 we had something like that before, but SonarQube has complained about unnecessary return; in the method. 🤷
The if (server != null) { also makes less code lines.
Related to: #10083
@NullMarkedto every packageBeanFactoryandApplicationEventPublisherare not null in TCP/IP components. Therefore, fix all the failing tests to ensure those properties and their propagation viaafterPropertiesSet()