Skip to content

[Merged by Bors] - Don't kill SSE stream if channel fills up#4500

Closed
michaelsproul wants to merge 1 commit intosigp:unstablefrom
michaelsproul:sse-reliability
Closed

[Merged by Bors] - Don't kill SSE stream if channel fills up#4500
michaelsproul wants to merge 1 commit intosigp:unstablefrom
michaelsproul:sse-reliability

Conversation

@michaelsproul
Copy link
Member

@michaelsproul michaelsproul commented Jul 13, 2023

Issue Addressed

Closes #4245

Proposed Changes

  • If an SSE channel fills up, send a comment instead of terminating the stream.
  • Add a CLI flag for scaling up the SSE buffer: --http-sse-capacity-multiplier N.

Additional Info

Blocked on #4462. I haven't rebased on that PR yet for initial testing, because it still needs some more work to handle long-running HTTP threads.

  • Add CLI flag tests.

@michaelsproul
Copy link
Member Author

Just pushed a new commit that's more aggressive at never erroring. I also removed the dumb warp backtracking behaviour, which might have been interfering (related: #3404).

So far I haven't gotten any disconnects from this updated commit.

@Savid
Copy link

Savid commented Jul 14, 2023

feedback from my local testing is that this is not disconnecting any more and I'm logging ~17-20k attestations per slot subscribed to the sse attestation topic. I also tested current sigp/lighthouse stable and started getting disconnected every few seconds (or when bursts of attestations come in).

@michaelsproul
Copy link
Member Author

@Savid YES! That's what I wanted to hear 🎉

@michaelsproul
Copy link
Member Author

Will block this on #4595 which gives us some of the nicer error handling for free

@michaelsproul michaelsproul added v4.4.1 ETA August 2023 ready-for-review The code is ready for review HTTP-API and removed blocked work-in-progress PR is a work-in-progress labels Aug 10, 2023
@michaelsproul
Copy link
Member Author

This is ready to go now

Copy link
Member

@paulhauner paulhauner left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@paulhauner paulhauner added ready-for-merge This PR is ready to merge. and removed ready-for-review The code is ready for review labels Aug 16, 2023
@michaelsproul
Copy link
Member Author

bors r+

bors bot pushed a commit that referenced this pull request Aug 17, 2023
## Issue Addressed

Closes #4245

## Proposed Changes

- If an SSE channel fills up, send a comment instead of terminating the stream.
- Add a CLI flag for scaling up the SSE buffer: `--http-sse-capacity-multiplier N`.

## Additional Info

~~Blocked on #4462. I haven't rebased on that PR yet for initial testing, because it still needs some more work to handle long-running HTTP threads.~~

- [x] Add CLI flag tests.
@bors
Copy link

bors bot commented Aug 17, 2023

Build failed (retrying...):

bors bot pushed a commit that referenced this pull request Aug 17, 2023
## Issue Addressed

Closes #4245

## Proposed Changes

- If an SSE channel fills up, send a comment instead of terminating the stream.
- Add a CLI flag for scaling up the SSE buffer: `--http-sse-capacity-multiplier N`.

## Additional Info

~~Blocked on #4462. I haven't rebased on that PR yet for initial testing, because it still needs some more work to handle long-running HTTP threads.~~

- [x] Add CLI flag tests.
@bors
Copy link

bors bot commented Aug 17, 2023

Build failed (retrying...):

@michaelsproul
Copy link
Member Author

bors r-

@bors
Copy link

bors bot commented Aug 17, 2023

Canceled.

@michaelsproul
Copy link
Member Author

bors r+

bors bot pushed a commit that referenced this pull request Aug 17, 2023
## Issue Addressed

Closes #4245

## Proposed Changes

- If an SSE channel fills up, send a comment instead of terminating the stream.
- Add a CLI flag for scaling up the SSE buffer: `--http-sse-capacity-multiplier N`.

## Additional Info

~~Blocked on #4462. I haven't rebased on that PR yet for initial testing, because it still needs some more work to handle long-running HTTP threads.~~

- [x] Add CLI flag tests.
@bors
Copy link

bors bot commented Aug 17, 2023

@bors bors bot changed the title Don't kill SSE stream if channel fills up [Merged by Bors] - Don't kill SSE stream if channel fills up Aug 17, 2023
@bors bors bot closed this Aug 17, 2023
@michaelsproul michaelsproul deleted the sse-reliability branch August 17, 2023 03:41
Woodpile37 pushed a commit to Woodpile37/lighthouse that referenced this pull request Jan 6, 2024
Closes sigp#4245

- If an SSE channel fills up, send a comment instead of terminating the stream.
- Add a CLI flag for scaling up the SSE buffer: `--http-sse-capacity-multiplier N`.

~~Blocked on sigp#4462. I haven't rebased on that PR yet for initial testing, because it still needs some more work to handle long-running HTTP threads.~~

- [x] Add CLI flag tests.
Woodpile37 pushed a commit to Woodpile37/lighthouse that referenced this pull request Jan 6, 2024
Closes sigp#4245

- If an SSE channel fills up, send a comment instead of terminating the stream.
- Add a CLI flag for scaling up the SSE buffer: `--http-sse-capacity-multiplier N`.

~~Blocked on sigp#4462. I haven't rebased on that PR yet for initial testing, because it still needs some more work to handle long-running HTTP threads.~~

- [x] Add CLI flag tests.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

HTTP-API ready-for-merge This PR is ready to merge. v4.4.1 ETA August 2023

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants