search

apache / pekko-http

The Streaming-first HTTP server/module of Apache Pekko
https://pekko.apache.org/
Apache License 2.0
150 stars 36 forks source link

Increase stream-cancellation-delay default to 1000 millis #590

Closed mdedetrich closed 1 week ago

mdedetrich commented 1 week ago

References: https://github.com/apache/pekko-http/issues/422

Still need to update tests as per @jrudolph original comment, i.e.

That broke lots of tests that probably didn't automatically propagate closing so they now fail while waiting for the now ten times longer cancellation times... (Might still might sense to "just" fix those tests)

pjfanning commented 1 week ago

Reading the Akka issue and PR seems to indicate that this will cause test failures. I can help if needed to try to fix up the test issues. I'm hoping to get Pekko HTTP 1.1.0 released but it seems useful to fix this.

mdedetrich commented 1 week ago

Reading the Akka issue and PR seems to indicate that this will cause test failures. I can help if needed to try to fix up the test issues. I'm hoping to get Pekko HTTP 1.1.0 released but it seems useful to fix this.

The test failures are expected because some of the tests expect the stream to be cancelled within 100ms and now that is has been increased to 1000ms those tests are now broken (as per @jrudolph 's comment, the change itself is safe).

mdedetrich commented 1 week ago

Ill have time to look into this tonight

JD557 commented 1 week ago

Should this actually be marked as closing the issue? It's better than the current status quo, but the underlying issue is still there.

For reference, we've used 1s for a long time with no issue until today, where we saw this resurface again. So 1s is not bullet proof.

mdedetrich commented 1 week ago

Should this actually be marked as closing the issue? It's better than the current status quo, but the underlying issue is still there.

For reference, we've used 1s for a long time with no issue until today, where we saw this resurface again. So 1s is not bullet proof.

Fair point, ill change this so it won't close original issue

mdedetrich commented 1 week ago

PR is ready, I managed to fix the issues with the tests.

mdedetrich commented 1 week ago

@pjfanning Feel free to merge when you see fit