Skip to content

Conversation

@yorkie
Copy link
Member

@yorkie yorkie commented Jul 17, 2025

…ect context

@yorkie yorkie requested a review from Copilot July 17, 2025 08:16
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR fixes command buffer response dispatching to ensure responses are delivered to the correct context. The change addresses issues where command buffer responses were not being properly matched to their originating contexts and request IDs.

Key changes include:

  • Modified command buffer response handling to include context and request ID matching
  • Refactored async response processing with improved synchronization mechanisms
  • Updated API signatures to require context and request ID parameters for proper response routing

Reviewed Changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.

File Description
src/client/per_process.hpp Updates typedef names, method signatures, and adds new data structures for context-aware response handling
src/client/per_process.cpp Implements new response matching logic with pending response management and improved worker thread handling
src/client/graphics/webgl_context.hpp Updates template method signatures to include request matching parameters
src/client/graphics/webgl_context.cpp Updates all method calls to use new context-aware response handling APIs
Comments suppressed due to low confidence (9)

src/client/per_process.hpp:316

  • Method name 'selectCommandbufferResponse' should use consistent camelCase formatting. It should be 'selectCommandBufferResponse' to match the naming pattern used elsewhere in the codebase.
  [[nodiscard]] TrCommandBufferResponse *selectCommandbufferResponse(client_graphics::WebGLContext *, int requestId);

src/client/per_process.hpp:520

  • Variable name 'commandbufferResponseWorker' should use consistent camelCase formatting. It should be 'commandBufferResponseWorker' to match the naming pattern used elsewhere in the codebase.
  unique_ptr<WorkerThread> commandbufferResponseWorker = nullptr;

src/client/per_process.hpp:521

  • Variable name 'pendingCommandbufferResponses' should use consistent camelCase formatting. It should be 'pendingCommandBufferResponses' to match the naming pattern used elsewhere in the codebase.
  vector<TrCommandBufferResponse *> pendingCommandbufferResponses;

src/client/per_process.hpp:529

  • Variable name 'asyncCommandbufferResponseCallbacks' should use consistent camelCase formatting. It should be 'asyncCommandBufferResponseCallbacks' to match the naming pattern used elsewhere in the codebase.
  vector<AsyncCommandBufferResponseCallback> asyncCommandbufferResponseCallbacks;

src/client/per_process.hpp:531

  • Variable name 'commandbufferResponseCv' should use consistent camelCase formatting. It should be 'commandBufferResponseCv' to match the naming pattern used elsewhere in the codebase.
  condition_variable commandbufferResponseCv;

src/client/per_process.hpp:532

  • Variable name 'mutexForCommandbufferResponses' should use consistent camelCase formatting. It should be 'mutexForCommandBufferResponses' to match the naming pattern used elsewhere in the codebase.
  mutex mutexForCommandbufferResponses;

src/client/per_process.hpp:533

  • Variable name 'mutexForAsyncCommandbufferResponseCallbacks' should use consistent camelCase formatting. It should be 'mutexForAsyncCommandBufferResponseCallbacks' to match the naming pattern used elsewhere in the codebase.
  shared_mutex mutexForAsyncCommandbufferResponseCallbacks;

src/client/per_process.cpp:491

  • Thread name 'TrCommandbufferResponseWorker' should use consistent camelCase formatting. It should be 'TrCommandBufferResponseWorker' to match the naming pattern used elsewhere in the codebase.
    commandbufferResponseWorker = make_unique<WorkerThread>("TrCommandbufferResponseWorker",

src/client/per_process.cpp:675

  • Method name 'selectCommandbufferResponse' should use consistent camelCase formatting. It should be 'selectCommandBufferResponse' to match the naming pattern used elsewhere in the codebase.
TrCommandBufferResponse *TrClientContextPerProcess::selectCommandbufferResponse(client_graphics::WebGLContext *context,

{
asyncResponseCallback.call(*resp);
it = asyncCommandbufferResponseCallbacks.erase(it);
delete resp; // End the response
Copy link

Copilot AI Jul 17, 2025

Choose a reason for hiding this comment

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

Early return without unlocking the shared_mutex could lead to resource leaks. The unique_lock should be explicitly released or the scope should be restructured to ensure proper cleanup.

Suggested change
delete resp; // End the response
delete resp; // End the response
lock.unlock(); // Explicitly release the lock

Copilot uses AI. Check for mistakes.
@yorkie yorkie merged commit 8a78bc8 into main Jul 17, 2025
2 checks passed
@yorkie yorkie deleted the fix/client-graphics-commandbuffers-resp branch July 17, 2025 09:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants