Fixes #1694: Populated the SOURCE_HOST vanflow attribute in the http2… #1695
Conversation
…e in the http2 adaptor
src/observers/http2/http2_observer.c
Outdated
| qd_http2_stream_info_t *stream_info = 0; | ||
| qd_error_t error = get_stream_info_from_hashtable(transport_handle, &stream_info, stream_id); | ||
| assert(stream_info != 0); | ||
| if(error == QD_ERROR_NOT_FOUND) { |
There was a problem hiding this comment.
Question that has nothing to do with this patch: should failure to find the given stream cause the observer to disable itself? Looks like we assert if this happens in a debug build so why continue decoding in a production build?
There was a problem hiding this comment.
Why is there an assert statement here? Does null stream_info indicate a software failure or could it be some kind of protocol anomaly. If it's the latter, crashing (even in a debug build) is unacceptable.
There was a problem hiding this comment.
should failure to find the given stream cause the observer to disable itself?
I am thinking aloud here. Say there are 100 streams in a http2 request and the first stream id is not found, would it be ok to continue to report on the other 99 streams instead of calling qdpo_http2_final() ?
| if(error == QD_ERROR_NOT_FOUND) { | |
| if (!stream_info) | |
| return; | |
| if(error == QD_ERROR_NOT_FOUND) { |
There was a problem hiding this comment.
kind of protocol anomaly
I would like to keep the assert in the debug build just to so we can get a chance to see why the issue is happening. But in production, we should just return if the stream_info is not found.
There was a problem hiding this comment.
Also, 'if' is not a function call. Please put a space after it.
There was a problem hiding this comment.
kind of protocol anomaly
I would like to keep the assert in the debug build just to so we can get a chance to see why the issue is happening. But in production, we should just return if the stream_info is not found.
If you're using the assert to look for a bug that you're seeing, convert this to a draft PR and remove the assert after you solve the problem.
I would expect that fuzz testing might cause that assert to fire. I don't think we want the router crashing in test due to known protocol anomalies.
… adaptor