You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@qpid.apache.org by GitBox <gi...@apache.org> on 2020/11/19 15:37:38 UTC
[GitHub] [qpid-dispatch] ganeshmurthy opened a new pull request #925: DISPATCH-1849: Don't dereference a peer delivery that has already bee…
ganeshmurthy opened a new pull request #925:
URL: https://github.com/apache/qpid-dispatch/pull/925
…n released. Check the in_dlv_released flag before dereferencing the in_dlv
----------------------------------------------------------------
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
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@qpid.apache.org
For additional commands, e-mail: dev-help@qpid.apache.org
[GitHub] [qpid-dispatch] ganeshmurthy closed pull request #925: DISPATCH-1849: Don't dereference a peer delivery that has already bee…
Posted by GitBox <gi...@apache.org>.
ganeshmurthy closed pull request #925:
URL: https://github.com/apache/qpid-dispatch/pull/925
----------------------------------------------------------------
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
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@qpid.apache.org
For additional commands, e-mail: dev-help@qpid.apache.org
[GitHub] [qpid-dispatch] ted-ross commented on a change in pull request #925: DISPATCH-1849: Don't dereference a peer delivery that has already bee…
Posted by GitBox <gi...@apache.org>.
ted-ross commented on a change in pull request #925:
URL: https://github.com/apache/qpid-dispatch/pull/925#discussion_r527038602
##########
File path: src/adaptors/http2/http2_adaptor.c
##########
@@ -1308,9 +1305,28 @@ static void qdr_http_delivery_update(void *context, qdr_delivery_t *dlv, uint64_
qdr_http2_stream_data_t* stream_data = qdr_delivery_get_context(dlv);
if (!stream_data)
return;
+
+ qdr_http2_connection_t *conn = stream_data->session_data->conn;
+
+ //
+ // DISPATCH-1849: In the case of large messages, the final DATA frame arriving from the server may or may not
+ // contain the END_STREAM flag. In the cases when the final DATA frame does not contain the END_STREAM flag,
+ // the router ends up forwarding all the data to the curl client without sending the END_STREAM to the client. The END_STREAM does arrive from the server
+ // but not before the curl client closes the client connection after receiving all the data. The curl client
+ // does not wait for the router to send an END_STREAM flag to close the connection. The client connection closure
+ // triggers the link cleanup on the ingress connection, in turn freeing up all deliveries and its peer deliveries.
+ // The peer delivery is released while it is still receiving the END_STREAM frame and the router crashes. To solve this issue,
+ // the stream_data->in_dlv_released flag is set to true when the peer delivery is released and this flag is
+ // check when performing further actions on the delivery. No action on the peer delivery is performed
+ // if this flag is set because the delivery and its underlying message have been freed.
Review comment:
Why don't you use the delivery ref-count mechanism to protect the delivery rather than introducing an additional flag?
----------------------------------------------------------------
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
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@qpid.apache.org
For additional commands, e-mail: dev-help@qpid.apache.org
[GitHub] [qpid-dispatch] ted-ross commented on a change in pull request #925: DISPATCH-1849: Don't dereference a peer delivery that has already bee…
Posted by GitBox <gi...@apache.org>.
ted-ross commented on a change in pull request #925:
URL: https://github.com/apache/qpid-dispatch/pull/925#discussion_r527042355
##########
File path: src/adaptors/http2/http2_adaptor.c
##########
@@ -1308,9 +1305,28 @@ static void qdr_http_delivery_update(void *context, qdr_delivery_t *dlv, uint64_
qdr_http2_stream_data_t* stream_data = qdr_delivery_get_context(dlv);
if (!stream_data)
return;
+
+ qdr_http2_connection_t *conn = stream_data->session_data->conn;
+
+ //
+ // DISPATCH-1849: In the case of large messages, the final DATA frame arriving from the server may or may not
+ // contain the END_STREAM flag. In the cases when the final DATA frame does not contain the END_STREAM flag,
+ // the router ends up forwarding all the data to the curl client without sending the END_STREAM to the client. The END_STREAM does arrive from the server
+ // but not before the curl client closes the client connection after receiving all the data. The curl client
+ // does not wait for the router to send an END_STREAM flag to close the connection. The client connection closure
+ // triggers the link cleanup on the ingress connection, in turn freeing up all deliveries and its peer deliveries.
+ // The peer delivery is released while it is still receiving the END_STREAM frame and the router crashes. To solve this issue,
+ // the stream_data->in_dlv_released flag is set to true when the peer delivery is released and this flag is
+ // check when performing further actions on the delivery. No action on the peer delivery is performed
+ // if this flag is set because the delivery and its underlying message have been freed.
+ //
+ if (settled && !conn->ingress && disp == PN_RELEASED) {
+ stream_data->in_dlv_released = true;
+ }
+
if (settled) {
nghttp2_nv hdrs[1];
- if (disp == PN_RELEASED || disp == PN_MODIFIED || disp == PN_REJECTED) {
+ if (conn->ingress && (disp == PN_RELEASED || disp == PN_MODIFIED || disp == PN_REJECTED)) {
if (disp == PN_RELEASED || disp == PN_MODIFIED) {
hdrs[0].name = (uint8_t *)":status";
hdrs[0].value = (uint8_t *)"503";
Review comment:
What happens on the server-side of the stream when the delivery is settled?
----------------------------------------------------------------
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
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@qpid.apache.org
For additional commands, e-mail: dev-help@qpid.apache.org