[GitHub] [trafficserver] maskit opened a new issue, #9992: Don't allocate Http3NoError

["maskit (via GitHub)" <gi...@apache.org>]

maskit opened a new issue, #9992:
URL: https://github.com/apache/trafficserver/issues/9992

   There are many places that instantiates `Http3NoError` but we shouldn't do it. Use of `std::option` without value should work for many cases.


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: issues-unsubscribe@trafficserver.apache.org.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [trafficserver] bneradt commented on issue #9992: Don't allocate Http3NoError

["bneradt (via GitHub)" <gi...@apache.org>]

bneradt commented on issue #9992:
URL: https://github.com/apache/trafficserver/issues/9992#issuecomment-1670377875

   Reading the code, the design is for polymorphism in which various functions return a `Http3Error` base class (actually its `Http3ErrorUPtr` unique_ptr type). When there is nothing wrong, they return the `Http3NoError` child class, as observed with this ticket. I don't recommend we mix dynamic dispatch via the pointer and std::optional. It's potentially confusing: are we communicating that the std::optional underlying pointer type could be nullptr? That is, someone might reasonably ask whether there is a difference between an empty std::optional error and a std::optional containing a value which is itself a nullptr.
   
   I therefore suggest one of the following:
   
   1. Stick with the setup as we have it, but do away with `Http3NoError` and instead return nullptr for those non-error situations. Or,
   2. Convert our Http3Error setup to a single type with an enum type describing the error, or lack of error, rather than using dynamic dispatch. The class is small enough, and errors presumably rare enough, that simply creating them as values and passing them by copy should be cheap. And likely even elided in many of the returns. 
   3. Same as 2, but do away with the `Http3NoError` concept and have things return a `std::optional<Http3Error>`.
   
   Any of these options seem fine to me. The simplest to implement at the moment is option 1, but I'm willing to do any. @maskit : you have preferences? (Or maybe feedback that I'm missing something?)


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: issues-unsubscribe@trafficserver.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [trafficserver] bneradt closed issue #9992: Don't allocate Http3NoError

["bneradt (via GitHub)" <gi...@apache.org>]

bneradt closed issue #9992: Don't allocate Http3NoError
URL: https://github.com/apache/trafficserver/issues/9992


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: issues-unsubscribe@trafficserver.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [trafficserver] maskit commented on issue #9992: Don't allocate Http3NoError

["maskit (via GitHub)" <gi...@apache.org>]

maskit commented on issue #9992:
URL: https://github.com/apache/trafficserver/issues/9992#issuecomment-1670451988

   What I probably wanted to do is avoiding nullptr checks, and `std::optional` was probably unavailable on ATS source code when I wrote the classes.
   
   - Http3NoError: Do not send anything
   - Http3ConnectionError: Send a connection error (the error code can be H3_NO_ERROR)
   - Http3StreamError (does not exist somehow): Send a stream error (the error code can be H3_NO_ERROR)
   
   But this (unexpectedly) required instantiating Http3NoError a lot of places. That's the background.
   
   So, the combination of `std::unique_ptr` and `nullptr` (i.e. the suggested option 1) is a simple possible solution. I'm fine with it. I just thought use of `std::optional` is a modern way, and I don't have a strong opinion on the use of `std::optional`. I agree that the combination of `std:unique_ptr` (which can have nullptr) and `std::optional` is confusing.
   
   Option 2 (and 3) may not be a great idea because Http3Error can contain a message string (the length is unlimited). The string has to be handed over to QUIC layer later, so managing the ownership (and the life time) of the string can be tricky if we copy errors.


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: issues-unsubscribe@trafficserver.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [trafficserver] bneradt commented on issue #9992: Don't allocate Http3NoError

["bneradt (via GitHub)" <gi...@apache.org>]

bneradt commented on issue #9992:
URL: https://github.com/apache/trafficserver/issues/9992#issuecomment-1670601706

   Sounds good. Thanks for the feedback and the context, @maskit. I think I'll go ahead and pursue option 1 then.


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: issues-unsubscribe@trafficserver.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org