Skip to content

Conversation

@mdaneri
Copy link
Contributor

@mdaneri mdaneri commented Mar 30, 2025

Description:
This pull request addresses issue [#1527] by modifying the way buffers are managed in PodeSignalRequest. The problem was that the base class, PodeRequest, uses a readonly, preallocated buffer that cannot be reinitialized. This caused issues for WebSocket signals (handled by PodeSignalRequest) where a fresh buffer is needed per Receive call.

Changes Introduced:

  1. Lazy Buffer Initialization in PodeRequest:

    • Removed the immediate allocation of the buffer from the constructor.
    • Introduced lazy initialization of the buffer using a private backing field with a property:
      private byte[] _buffer;
      protected virtual byte[] Buffer
      {
          get
          {
              if (_buffer == null)
              {
                  _buffer = new byte[BufferSize];
              }
              return _buffer;
          }
      }
    • Added a virtual method GetBuffer() that returns the buffer. By default, it returns the lazily initialized, single preallocated buffer:
      protected virtual byte[] GetBuffer() => Buffer;
  2. Override in PodeSignalRequest:

    • In PodeSignalRequest, override GetBuffer() to return a new byte array for each Receive call:
      protected override byte[] GetBuffer() => new byte[BufferSize];

    This ensures that every WebSocket signal request gets a fresh buffer, avoiding any leftover data from previous operations.

  3. Integration Test:

    • Added a Pester integration test that creates a WebSocket client to send a JSON signal and asserts that the returned message (e.g. a date string) is correctly received. This test mimics the client behavior from our JS code and ensures that the buffer management in PodeSignalRequest is working correctly.

Impact:

  • These changes improve memory management by only allocating the shared buffer when necessary for most requests.
  • For WebSocket signals, a new buffer is used on each receive call, ensuring data integrity.
  • The modifications are backward-compatible for other request types (e.g., HTTP requests) that benefit from reusing the preallocated buffer.

Testing:

  • Verified the changes locally using the provided Pester integration test.
  • The WebSocket client test successfully sends a signal and receives the correct response without any buffer contamination issues.

Overridden GetBuffer() in PodeSignalRequest to return a new buffer per receive call, ensuring WebSocket signals use a fresh buffer and resolving issue Badgerati#1527. The base class now lazily initializes the shared buffer for non-WebSocket requests.
@mdaneri
Copy link
Contributor Author

mdaneri commented Apr 11, 2025

Added a note to the documentation regarding the current limit of 32KB payload

@Badgerati Badgerati linked an issue Apr 12, 2025 that may be closed by this pull request
@Badgerati Badgerati changed the title Fix buffer reuse issue in PodeSignalRequest by overriding GetBuffer Fix buffer reuse issue in WebSockets Apr 12, 2025
@Badgerati Badgerati added this to the 2.12.1 milestone Apr 12, 2025
Copy link
Owner

@Badgerati Badgerati left a comment

Choose a reason for hiding this comment

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

LGTM

@Badgerati Badgerati merged commit 980d899 into Badgerati:develop Apr 12, 2025
13 checks passed
@mdaneri mdaneri deleted the issue-1527 branch April 12, 2025 16:02
@Badgerati Badgerati mentioned this pull request Apr 27, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Web socket regression on 2.12.0

2 participants