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

In example: Error and note #2739

Closed
heretic13 opened this issue Sep 13, 2023 · 9 comments · Fixed by #2784
Closed

In example: Error and note #2739

heretic13 opened this issue Sep 13, 2023 · 9 comments · Fixed by #2784
Assignees
Labels
Example An issue which concerns the examples

Comments

@heretic13
Copy link

Version of Beast

At Boost 1_83.

I have some comments about this example.
https://www.boost.org/doc/libs/1_83_0/libs/beast/example/advanced/server/advanced_server.cpp

  1. Note.

In function
void queue_write(http::message_generator response)
You do push_back first.
You then ensure that no work is done by checking size()==1 for the vector.

Abstracting from the container type, the empty() operation takes the same amount of time as size() or faster. It is more profitable to first check the vector for empty() and store the result in a variable. Then do push_back(). Then, depending on the value of the variable, execute do_write();

  1. Error.
    In function
    bool do_write()

After checking !response_queue_.empty(), you retrieve the element from the vector and start the asynchronous operation.

If you look at the body of the queue_write() function, you can see that it controls the absence of an asynchronous operation by comparing the queue size with 1 (after adding an element).
Which is incorrect.

Example:

Start. queue size=0

  1. queue_write(). queue size=1
  2. call do_write()
    starting an asynchronous operation. queue size=0
  3. queue_write(). queue size=1
  4. call do_write()
    starting an asynchronous operation. queue size=0

We did not wait for the previous asynchronous operation to complete and started a new one.

@heretic13
Copy link
Author

  1. Error.

Function
// Returns true if the caller may initiate a new read
bool do_write()

Further in the text, the function returns the sign that the queue is completely full.

That is, if the queue is full, we can start a new reading??

@klemens-morgenstern
Copy link
Collaborator

  1. The queue_write checks if the queue was empty before being called - why is that wrong?
  2. Yes. What's the issue?

@heretic13
Copy link
Author

  1. The queue_write checks if the queue was empty before being called - why is that wrong?
  2. Yes. What's the issue?
  1. Documenatation
    https://www.boost.org/doc/libs/1_83_0/libs/beast/doc/html/beast/ref/boost__beast__async_write.html
    "The program must ensure that the stream performs no other writes until this operation completes."
    The example above CAN runs MULTIPLE async_write operations.

  2. In the real world, we make a queue to limit the amount of data sent.

If I correctly understood the author’s idea, he assumed a request->response sequence with a quantity limit. The request has been processed, put into the submission queue, and we are ready to accept a new request. We shouldn't accept a new request if we can't save the result to the queue.
This rule is broken here.

@klemens-morgenstern
Copy link
Collaborator

  1. How?
  2. Ok, so why's that an error?

@vinniefalco
Copy link
Member

vinniefalco commented Sep 14, 2023

if the queue is full we should not read the next request until there is room for a response, this will allow TCP/IP's natural application-level flow control to work properly

@heretic13
Copy link
Author

  1. How?

Read carefully. I gave an example above.
Example (repeat):

Start. queue size=0

  1. queue_write(). queue size=1
  2. call do_write()
    starting an asynchronous operation. queue size=0
  3. queue_write(). queue size=1
  4. call do_write()
    starting an asynchronous operation. queue size=0
    We did not wait for the previous asynchronous operation to complete and started a new one.
  1. Ok, so why's that an error?

Because in your (original) example we start a new read request when the send queue is full. This defeats the purpose of limiting the send queue.

@heretic13
Copy link
Author

if the queue is full we should not read the next request until there is room for a response, this will allow TCP/IP's natural application-level flow control to work properly

This is right. Good idea.
But this doesn't work in your example.

@ashtum ashtum self-assigned this Jan 1, 2024
@ashtum ashtum added the Example An issue which concerns the examples label Jan 1, 2024
@ashtum
Copy link
Collaborator

ashtum commented Jan 1, 2024

@heretic13, I can confirm that there is a bug in this line:

response_queue_.erase(response_queue_.begin());

This leads to a situation where we initiate another async_write while there is an ongoing one.

ashtum added a commit to ashtum/beast that referenced this issue Jan 1, 2024
ashtum added a commit to ashtum/beast that referenced this issue Jan 2, 2024
ashtum added a commit that referenced this issue Jan 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Example An issue which concerns the examples
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants