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

Actors - allow setting of timeout for opening remote Socket connections #2468

Closed
wants to merge 1 commit into from

Conversation

martinmcnulty
Copy link
Contributor

This is a fix for the issue raised by Chris Marshall in SI-5734 which should alleviate problems where opening a Socket to a remote Node blocks (for up to a minute in our experience) not allowing other remote messages to be sent during this time. An optional timeout can be set through a system property that is passed to the Socket when it is connected.

Default behaviour is unchanged, since the property defaults to 0, which signifies "no timeout" to the java.net.Socket.

@retronym
Copy link
Member

Thanks for the contribution.

Can you please squash the two commits into one and change the message to:

SI-5734 Allow setting of socket timeout for remote actors

Description of what was wrong before.

What you changed, and how that fixes the problem.

How to verify the change works.

More details on commit message style: https://github.com/scala/scala/blob/master/CONTRIBUTING.md

BTW, Scala Actors are in the process of deprecation in favour of Akka actors, so to accept contributions in this area, we need to make them easy to review (well documented/tested/etc.)

TcpService is locked while connections are being established, preventing
messages being sent to remote nodes. There was no way to specify a connect
timeout, so the service was waiting for a ConnectException to be thrown if
the remote node was unavailable (which can take several minutes).

Added a system property to allow setting a socket connect timeout to
prevent the TcpService from being locked for minutes at a time.

Default behaviour is unchanged, all tests pass and the patch has been
running in production without issue.
@martinmcnulty
Copy link
Contributor Author

Thanks @retronym, I've squashed the commits and updated the message. Sorry for not reading to the end of the guidelines before. I realise Scala Actors are deprecated but we have an app in production that would be difficult to migrate to Akka. This patch has fixed the problems we were having with it in production, so I thought I'd share it.

@adriaanm
Copy link
Contributor

adriaanm commented May 1, 2013

@martinmcnulty, thanks for your contribution! Unfortunately, there won't be another 2.10.1 release, so could you please target the 2.10.x branch instead? Thanks!

@adriaanm adriaanm closed this May 1, 2013
@martinmcnulty
Copy link
Contributor Author

@adriaanm - thanks. Reopened as #2480. I've targeted master, with a note to backport to 2.10.x because I think it should be included in future 2.11.x releases as well. Hope that's ok.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants