mdedetrich commented on code in PR #590:
URL: https://github.com/apache/pekko-http/pull/590#discussion_r1747149310
##########
http-core/src/test/resources/reference.conf:
##########
@@ -6,4 +6,5 @@ pekko {
default-dispatcher.throughput = 1
}
stream.materializer.debug.fuzzing-mode = off
+ stream.testkit.all-stages-stopped-timeout = 20 s
Review Comment:
> I cannot remember the reason to choose 100ms for the delay cancellation
limit. Having to increase the all-stages-stopped-timeout to higher values is
somewhat of an indication of what kind of real world implications this change
might have: cleanup of broken/closed connections might now take longer.
iirc its only in a few tests which used `Utils.assertAllStagesStopped`
started timing out so I can granularly update those tests. My ultimate
reasoning for making this change global is was to reduce surprises for people
writing future tests (i.e. giving more QoL to developers), more specifically
avoiding the "I just updated/wrote a test and now its failing due to a finely
tuned timeout and now I have to figure out that it was because of that said
timeout"
I think that as long as you handle shutdown of pekko http gracefully I don't
think it should cause issues in prod however I admit that many people may not
even be handling shutdowns of pekko http correctly.
> IIRC, the whole reason to introduce the delay was to deal with the fact
that stream cancellation could not be well-configured in Akka 2.5. With 2.6,
it's not 100% clear if the delay is even still needed or whether it could be
solved in a better way (i.e. by resolving the cancellation race more
intelligently, taking the cancellation reason into account at the right/more
places) to avoid symptoms as shown in the ticket.
If this is the case then it should definitely be looked into, I am not aware
of the updates to stream cancellation in akka 2.6 but judging by the date of
the report at https://github.com/apache/pekko-http/issues/422#issue-2084209600,
if that stream cancellation update is now the default it didn't seem to fix the
underlying issue.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
