You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@trafficserver.apache.org by GitBox <gi...@apache.org> on 2021/02/04 22:03:51 UTC
[GitHub] [trafficserver] traeak opened a new pull request #7486: slice/Range: change maxval to use defined value instead of numeric_limits
traeak opened a new pull request #7486:
URL: https://github.com/apache/trafficserver/pull/7486
using std::numeric_limits in a constexpr initialization is dubious at best. Convert to use hard coded value LLONG_MAX instead.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] traeak commented on pull request #7486: slice/Range: change maxval to use defined value instead of numeric_limits
Posted by GitBox <gi...@apache.org>.
traeak commented on pull request #7486:
URL: https://github.com/apache/trafficserver/pull/7486#issuecomment-774017236
Yes. Using devtoolset-7 on centos. Had quick sets of crashes on 2 machines (the first one had a bad cache drive). No crashes since, no crashes during canary.
From backtrace the observation was that the crashes in question always had:
&Range::maxval = &range.begin which shouldn't be possible.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] traeak edited a comment on pull request #7486: slice/server: handleFirstServerHeader exit sooner on detected requested range errors.
Posted by GitBox <gi...@apache.org>.
traeak edited a comment on pull request #7486:
URL: https://github.com/apache/trafficserver/pull/7486#issuecomment-774017236
Yes. Using devtoolset-7 on centos. Had quick sets of crashes on 2 machines (the first one had a bad cache drive). No crashes since, no crashes during canary.
From backtrace the observation was that the crashes in question always had:
&Range::maxval = &range.begin which shouldn't be possible.
Looks like the handleFirstServerHeader should return sooner when range errors are detected.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] bneradt commented on pull request #7486: slice/Range: change maxval to use defined value instead of numeric_limits
Posted by GitBox <gi...@apache.org>.
bneradt commented on pull request #7486:
URL: https://github.com/apache/trafficserver/pull/7486#issuecomment-773723000
I believe per the standard numeric_limits are constexpr. https://en.cppreference.com/w/cpp/types/numeric_limits/max lists it as constexpr. I'd say numeric_limits are more idiomatic for C++ than the _MAX macros.
Out of curiosity, @traeak , did you run into an issue with numeric_limits that motivated this?
Thanks,
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] zwoop commented on pull request #7486: slice/server: handleFirstServerHeader exit sooner on detected requested range errors.
Posted by GitBox <gi...@apache.org>.
zwoop commented on pull request #7486:
URL: https://github.com/apache/trafficserver/pull/7486#issuecomment-783517045
Cherry-picked to v9.0.x branch.
Cherry-picked to v9.1.x branch.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] traeak merged pull request #7486: slice/server: handleFirstServerHeader exit sooner on detected requested range errors.
Posted by GitBox <gi...@apache.org>.
traeak merged pull request #7486:
URL: https://github.com/apache/trafficserver/pull/7486
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] SolidWallOfCode commented on pull request #7486: slice/Range: change maxval to use defined value instead of numeric_limits
Posted by GitBox <gi...@apache.org>.
SolidWallOfCode commented on pull request #7486:
URL: https://github.com/apache/trafficserver/pull/7486#issuecomment-773720287
Why is that? I've never heard that claim before. Testing with g++ back to 8.3, `std::numeric_limits<in64_t>::max()` works as a `constexpr`, e.g. use of it causes a load immediate instruction, even at "-O0".
If you want to be absolute certain, do this:
```
#include <type_traits>
static_assert(std::integral_constant<int64_t, Range::maxval>::value);
```
If that compiles, it's a compile time constant.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] traeak edited a comment on pull request #7486: slice/Range: change maxval to use defined value instead of numeric_limits
Posted by GitBox <gi...@apache.org>.
traeak edited a comment on pull request #7486:
URL: https://github.com/apache/trafficserver/pull/7486#issuecomment-774017236
Yes. Using devtoolset-7 on centos. Had quick sets of crashes on 2 machines (the first one had a bad cache drive). No crashes since, no crashes during canary.
From backtrace the observation was that the crashes in question always had:
&Range::maxval = &range.begin which shouldn't be possible.
Looks like the first server header parser should exit earlier on detected 416 requests, adjusting.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] traeak commented on a change in pull request #7486: slice/server: handleFirstServerHeader exit sooner on detected requested range errors.
Posted by GitBox <gi...@apache.org>.
traeak commented on a change in pull request #7486:
URL: https://github.com/apache/trafficserver/pull/7486#discussion_r575453227
##########
File path: plugins/experimental/slice/Range.h
##########
@@ -29,7 +29,7 @@
struct Range {
public:
- static int64_t constexpr maxval = (std::numeric_limits<int64_t>::max() >> 2);
+ static int64_t constexpr maxval = (std::numeric_limits<int64_t>::max() / 2);
Review comment:
it just has to be sufficiently large but not max64.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] bneradt commented on pull request #7486: slice/Range: change maxval to use defined value instead of numeric_limits
Posted by GitBox <gi...@apache.org>.
bneradt commented on pull request #7486:
URL: https://github.com/apache/trafficserver/pull/7486#issuecomment-773723000
I believe per the standard numeric_limits are constexpr. https://en.cppreference.com/w/cpp/types/numeric_limits/max lists it as constexpr. I'd say numeric_limits are more idiomatic for C++ than the _MAX macros.
Out of curiosity, @traeak , did you run into an issue with numeric_limits that motivated this?
Thanks,
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] SolidWallOfCode commented on pull request #7486: slice/Range: change maxval to use defined value instead of numeric_limits
Posted by GitBox <gi...@apache.org>.
SolidWallOfCode commented on pull request #7486:
URL: https://github.com/apache/trafficserver/pull/7486#issuecomment-773720287
Why is that? I've never heard that claim before. Testing with g++ back to 8.3, `std::numeric_limits<in64_t>::max()` works as a `constexpr`, e.g. use of it causes a load immediate instruction, even at "-O0".
If you want to be absolute certain, do this:
```
#include <type_traits>
static_assert(std::integral_constant<int64_t, Range::maxval>::value);
```
If that compiles, it's a compile time constant.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] traeak edited a comment on pull request #7486: slice/server: handleFirstServerHeader exit sooner on detected requested range errors.
Posted by GitBox <gi...@apache.org>.
traeak edited a comment on pull request #7486:
URL: https://github.com/apache/trafficserver/pull/7486#issuecomment-774017236
Looks like the handleFirstServerHeader should return sooner when range errors are detected.
backtrace seemed to indicate that this static var pointed to the same address as the first data member in that Range instance. That sent me in the wrong direction. std::numeric_limits has sometimes not behaved not very well with msvc, especially when used for default arg values.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] ywkaras commented on a change in pull request #7486: slice/server: handleFirstServerHeader exit sooner on detected requested range errors.
Posted by GitBox <gi...@apache.org>.
ywkaras commented on a change in pull request #7486:
URL: https://github.com/apache/trafficserver/pull/7486#discussion_r575304109
##########
File path: plugins/experimental/slice/Range.h
##########
@@ -29,7 +29,7 @@
struct Range {
public:
- static int64_t constexpr maxval = (std::numeric_limits<int64_t>::max() >> 2);
+ static int64_t constexpr maxval = (std::numeric_limits<int64_t>::max() / 2);
Review comment:
It's not clear why this is being changed from max / 4 to max / 2, or really why it's not just max. But I'll take it on faith.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] traeak edited a comment on pull request #7486: slice/server: handleFirstServerHeader exit sooner on detected requested range errors.
Posted by GitBox <gi...@apache.org>.
traeak edited a comment on pull request #7486:
URL: https://github.com/apache/trafficserver/pull/7486#issuecomment-774017236
Looks like the handleFirstServerHeader should return sooner when range errors are detected.
backtrace seemed to indicate that this static var pointed to the same address as the first data member in that Range instance. That sent me in the wrong direction.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] ywkaras commented on a change in pull request #7486: slice/server: handleFirstServerHeader exit sooner on detected requested range errors.
Posted by GitBox <gi...@apache.org>.
ywkaras commented on a change in pull request #7486:
URL: https://github.com/apache/trafficserver/pull/7486#discussion_r575295364
##########
File path: plugins/experimental/slice/server.cc
##########
@@ -119,8 +117,7 @@ handleFirstServerHeader(Data *const data, TSCont const contp)
TSVIONBytesSet(output_vio, clen);
}
TSHttpHdrPrint(header.m_buffer, header.m_lochdr, output_buf);
- state = HeaderState::Passthru;
- return state;
+ return HeaderState::Passthru;
Review comment:
These probably get optimized out but it's worthwhile cleanup.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org