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

Addressed performance issue where connection would be closed every ti… #2453

Merged

Conversation

Projects
None yet
3 participants
@cmcmteixeira
Copy link
Contributor

cmcmteixeira commented Mar 16, 2019

…me and not only when an error ocurred.

Addressed performance issue where connection would be closed every ti…
…me and not only when an error ocurred.
@cmcmteixeira

This comment has been minimized.

Copy link
Contributor Author

cmcmteixeira commented Mar 16, 2019

I believe there could be a couple of issues with this code:

  1. I believe that if the stream is cancelled, stageShutdown could never run
  2. trailerCleanup(); cleanup(); will run after the stageShutdown() step

Not sure how relevant these constraints are unfortunately..

@cmcmteixeira cmcmteixeira force-pushed the cmcmteixeira:fix_performance_degradation branch from 60b1190 to ec8a32c Mar 16, 2019

@rossabaker

This comment has been minimized.

Copy link
Member

rossabaker commented Mar 18, 2019

I believe that if the stream is cancelled, stageShutdown could never run

onFinalizeCase should give the hook to run this. In practice, the client is likely to disconnect and trigger a shutdown, but we can be a bit more aggressive on our end.

trailerCleanup(); cleanup(); will run after the stageShutdown() step

I don't think this will hurt anything, but it doesn't even need to run at all. It could be another case in onFinalizeCase.

I think the logic could be:

            attributes -> rawBody
              .onFinalizeCase {
                case ExitCase.Completed =>
                  Async.shift(executionContext) *> F.delay { trailerCleanup(); cleanup() }
                case _ =>
                  Async.shift(executionContext) *> F.delay(stageShutdown())
              }
@cmcmteixeira

This comment has been minimized.

Copy link
Contributor Author

cmcmteixeira commented Mar 18, 2019

Ah! onFinalizeCase seems to be only part of the fs2 1.x series.
I can make another PR targeting Http4s 0.20 series with the onFinalizeCase but for the 0.18 it seems like we're stuck with having to to onFinalize + handleErrorWith;

@cmcmteixeira

This comment has been minimized.

Copy link
Contributor Author

cmcmteixeira commented Mar 18, 2019

Also, because of the limitation of only having onFinalize & handleErrorWith I can't think of a way to be able to run only stageShutdown in case of error and trailerCleanup+cleanup in case of success; they will both have to run in case of error..

@rossabaker

This comment has been minimized.

Copy link
Member

rossabaker commented Mar 18, 2019

Oh, I see. This PR targets master, so I thought you were giving up on the 0.18.x fix. That should target release-0.18.x.

I don't see any harm in running trailerCleanup and cleanup on error in 0.18.x. It's cleaning up state that will be discarded, but it shouldn't negatively affect semantics. The thing we must avoid is calling stageShutdown() on success.

@cmcmteixeira cmcmteixeira changed the base branch from master to release-0.18.x Mar 18, 2019

@cmcmteixeira

This comment has been minimized.

Copy link
Contributor Author

cmcmteixeira commented Mar 18, 2019

Ups, my bad! Fixed now 👍
I'll create a PR with the onFinalizeCase for 0.20x in the mean time to get this thing over with :)

@rossabaker rossabaker merged commit 3a2411a into http4s:release-0.18.x Mar 19, 2019

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.