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

Remove SocketUtils usage #3728

Merged
merged 1 commit into from
Feb 16, 2022
Merged

Remove SocketUtils usage #3728

merged 1 commit into from
Feb 16, 2022

Conversation

artembilan
Copy link
Member

@artembilan artembilan commented Feb 15, 2022 •
edited

  • Remove usage of non-stable org.springframework.util.SocketUtils
  • Replace it with 0 for those tests where it is possible to select OS port
  • Remove the mentioning of the SocketUtils from the testing.adoc
  • Use TransportConstants.DEFAULT_STOMP_PORT for StompServerIntegrationTests.
    We may disable this test in the future for CI if it is not going to be stable
  • Introduce Supplier<String> connectUrl variants for ZeroMqMessageHandler
    to let it defer connection evaluation until subscription to the socket Mono
    in the ZeroMqMessageHandler.
  • Move connection logic in the ZeroMqMessageHandler to Lifecycle.start()

Related to spring-projects/spring-framework#28054

Cherry-pick to 5.5.x

@artembilan
Remove SocketUtils usage
fb72f1f 
* Remove usage of non-stable `org.springframework.util.SocketUtils`
* Replace it with `0` for those tests where it is possible to select OS port
* Remove the mentioning of the `SocketUtils` from the `testing.adoc`
* Use `TransportConstants.DEFAULT_STOMP_PORT` for `StompServerIntegrationTests`.
We may disable this test in the future for CI if it is not going to be stable
* Introduce `Supplier<String> connectUrl` variants for `ZeroMqMessageHandler`
to let it defer connection evaluation until subscription to the socket `Mono`
in the `ZeroMqMessageHandler`.
* Move connection logic in the `ZeroMqMessageHandler` to `Lifecycle.start()`

Related to spring-projects/spring-framework#28054

**Cherry-pick to `5.4.x`**
@artembilan artembilan added this to the 6.0 M2 milestone Feb 15, 2022
@artembilan
Copy link
Member Author

@garyrussell ,

I'd appreciate if you test it locally on your OS.
Not sure why we have used an explicit port for those UDP tests all this time...

Thanks

garyrussell
garyrussell suggested changes Feb 15, 2022
View reviewed changes
Copy link
Contributor

@garyrussell garyrussell left a comment

Choose a reason for hiding this comment

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

Unfortunately, UDP doesn't work on my new MacBook and I haven't had time to figure out why, but our friends at GH Actions have verified it.

I don't see how we can cherry-pick this (except just the tests) because it is a documented class - it needs to be deprecated in 5.5.x.

.../src/test/java/org/springframework/integration/stomp/client/StompServerIntegrationTests.java
ConfigurationImpl configuration =
new ConfigurationImpl()
.setName("embedded-server")
.setPersistenceEnabled(false)
.setSecurityEnabled(false)
.setJMXManagementEnabled(false)
.setJournalDatasync(false)
.addAcceptorConfiguration("stomp", "tcp://127.0.0.1:" + port)
.addAcceptorConfiguration("stomp", "tcp://127.0.0.1:" + TransportConstants.DEFAULT_STOMP_PORT)
Copy link
Contributor

Choose a reason for hiding this comment

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

It is better to use a flaky SocketUtils rather than hard coding to a single port if the component doesn't support 0.

I'd prefer disabling (per your comment), if you want to remove it completely.

Copy link
Member Author

Choose a reason for hiding this comment

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

There is no such a class in SF 6.0 any more.
So, I wouldn't mind to watch this until it fails with the port conflict 😄

@artembilan
Copy link
Member Author

it needs to be deprecated in 5.5.x.

There is nothing to deprecate for us: we use already that one from Spring Framework.
It is used only in the tests therefore the cherry-pick it straight forward without any impact on the production code.
Plus that ZeroMQ has gotten a new feature to determine the port on demand 😄 .

@garyrussell garyrussell merged commit c661d79 into spring-projects:main Feb 16, 2022
@garyrussell
Copy link
Contributor

Stomp needs back-porting for 5.5.x; Artemis is not present there.

@artembilan
Copy link
Member Author

Thanks. On it.

@artembilan artembilan deleted the remove_SocketUtils branch February 16, 2022 14:17
@artembilan
Copy link
Member Author

... and back-ported to 5.5.x as 9b27fbe after fixing conflicts in the STOMP test.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@garyrussell garyrussell garyrussell approved these changes

Assignees
No one assigned
Labels
in: testing type: refactoring
Projects
None yet
Milestone
6.0 M2
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@artembilan @garyrussell