You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@pekko.apache.org by "nvollmar (via GitHub)" <gi...@apache.org> on 2024/03/07 09:48:49 UTC
[PR] Add handling for tcp register timeout leaving connection dead [incubator-pekko]
nvollmar opened a new pull request, #1183:
URL: https://github.com/apache/incubator-pekko/pull/1183
Refs #1182
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
Re: [PR] Add handling for tcp register timeout leaving connection dead [incubator-pekko]
Posted by "Roiocam (via GitHub)" <gi...@apache.org>.
Roiocam commented on code in PR #1183:
URL: https://github.com/apache/incubator-pekko/pull/1183#discussion_r1516114887
##########
actor/src/main/scala/org/apache/pekko/io/dns/internal/TcpDnsClient.scala:
##########
@@ -80,7 +80,10 @@ import pekko.util.ByteString
}
}
case Tcp.PeerClosed =>
+ context.unwatch(connection)
context.become(idle)
+ case Terminated(`connection`) =>
+ throwFailure("TCP connection terminated without closing (register timeout?)", None)
Review Comment:
Did you notice `TcpOutgoingConnection` doing the same thing here?
https://github.com/apache/incubator-pekko/blob/46e60a61fbabce5e3f36a408bfa3d1fb249eef44/actor/src/main/scala/org/apache/pekko/io/TcpOutgoingConnection.scala#L58-L59
It will send `CommandFailed` to `commander(TcpDnsClient)`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
Re: [PR] Add handling for tcp register timeout leaving connection dead [incubator-pekko]
Posted by "nvollmar (via GitHub)" <gi...@apache.org>.
nvollmar commented on code in PR #1183:
URL: https://github.com/apache/incubator-pekko/pull/1183#discussion_r1516139264
##########
actor/src/main/scala/org/apache/pekko/io/dns/internal/TcpDnsClient.scala:
##########
@@ -80,7 +80,10 @@ import pekko.util.ByteString
}
}
case Tcp.PeerClosed =>
+ context.unwatch(connection)
context.become(idle)
+ case Terminated(`connection`) =>
+ throwFailure("TCP connection terminated without closing (register timeout?)", None)
Review Comment:
Not in the case of the register timeout, there it will just do `context.stop`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
Re: [PR] Add handling for tcp register timeout leaving connection dead [incubator-pekko]
Posted by "Roiocam (via GitHub)" <gi...@apache.org>.
Roiocam commented on code in PR #1183:
URL: https://github.com/apache/incubator-pekko/pull/1183#discussion_r1516225790
##########
actor/src/main/scala/org/apache/pekko/io/dns/internal/TcpDnsClient.scala:
##########
@@ -80,7 +80,10 @@ import pekko.util.ByteString
}
}
case Tcp.PeerClosed =>
+ context.unwatch(connection)
context.become(idle)
+ case Terminated(`connection`) =>
+ throwFailure("TCP connection terminated without closing (register timeout?)", None)
Review Comment:
Ahh, I see. The connection is ready, and there are only the last two Acks left. Things from TcpOutgoingConnection to TcpConnection.
Thanks for your explanation.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
Re: [PR] Add handling for tcp register timeout leaving connection dead [incubator-pekko]
Posted by "nvollmar (via GitHub)" <gi...@apache.org>.
nvollmar commented on code in PR #1183:
URL: https://github.com/apache/incubator-pekko/pull/1183#discussion_r1516150554
##########
actor/src/main/scala/org/apache/pekko/io/dns/internal/TcpDnsClient.scala:
##########
@@ -49,6 +48,7 @@ import pekko.util.ByteString
log.debug("Connected to TCP address [{}]", ns)
val connection = sender()
context.become(ready(connection))
+ context.watch(connection)
Review Comment:
Other usages of that connection also watch it to fail on termination:
https://github.com/apache/incubator-pekko/blob/46e60a61fbabce5e3f36a408bfa3d1fb249eef44/stream/src/main/scala/org/apache/pekko/stream/impl/io/TcpStages.scala#L295-L296
That's why I did the same here for a low risk fix of the issue.
In general it would make sense to rethink the construct as a whole as the code is more or less unchanged since at least 2013.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
Re: [PR] Add handling for tcp register timeout leaving connection dead [incubator-pekko]
Posted by "mdedetrich (via GitHub)" <gi...@apache.org>.
mdedetrich commented on PR #1183:
URL: https://github.com/apache/incubator-pekko/pull/1183#issuecomment-1986811038
Got enough approvals, merging so it can get released with M1
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
Re: [PR] Add handling for tcp register timeout leaving connection dead [incubator-pekko]
Posted by "pjfanning (via GitHub)" <gi...@apache.org>.
pjfanning commented on PR #1183:
URL: https://github.com/apache/incubator-pekko/pull/1183#issuecomment-1983309638
maybe in some further iteration, we could look at retries - but this is an improvement as is
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
Re: [PR] Add handling for tcp register timeout leaving connection dead [incubator-pekko]
Posted by "mdedetrich (via GitHub)" <gi...@apache.org>.
mdedetrich merged PR #1183:
URL: https://github.com/apache/incubator-pekko/pull/1183
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
Re: [PR] Add handling for tcp register timeout leaving connection dead [incubator-pekko]
Posted by "He-Pin (via GitHub)" <gi...@apache.org>.
He-Pin commented on PR #1183:
URL: https://github.com/apache/incubator-pekko/pull/1183#issuecomment-1983472222
I will TAL at weekend, a little busy at $Work
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
Re: [PR] Add handling for tcp register timeout leaving connection dead [incubator-pekko]
Posted by "nvollmar (via GitHub)" <gi...@apache.org>.
nvollmar commented on PR #1183:
URL: https://github.com/apache/incubator-pekko/pull/1183#issuecomment-1983324217
I was thinking going the other direction and questioning the need for the `Register` message.
As far as I could see from a quick search, the `Register` is always sent by the same actor that opens the connections.
So do we need to have that capability to have a different handler? Or could we inline everything into the connection open and avoid having that timeout entirely?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
Re: [PR] Add handling for tcp register timeout leaving connection dead [incubator-pekko]
Posted by "Roiocam (via GitHub)" <gi...@apache.org>.
Roiocam commented on code in PR #1183:
URL: https://github.com/apache/incubator-pekko/pull/1183#discussion_r1516106276
##########
actor/src/main/scala/org/apache/pekko/io/dns/internal/TcpDnsClient.scala:
##########
@@ -49,6 +48,7 @@ import pekko.util.ByteString
log.debug("Connected to TCP address [{}]", ns)
val connection = sender()
context.become(ready(connection))
+ context.watch(connection)
Review Comment:
I noticed that the connection will also watch the current actor. Does the actor of each other's watch make sense?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
Re: [PR] Add handling for tcp register timeout leaving connection dead [incubator-pekko]
Posted by "laglangyue (via GitHub)" <gi...@apache.org>.
laglangyue commented on PR #1183:
URL: https://github.com/apache/incubator-pekko/pull/1183#issuecomment-1983321444
LGTM
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org