Skip to content
This repository was archived by the owner on Nov 15, 2023. It is now read-only.

Conversation

@mxinden
Copy link
Contributor

@mxinden mxinden commented Jan 22, 2021

RequestId is a monotonically increasing integer, starting at
1. A RequestId is unique for a single RequestResponse
behaviour, but not across multiple RequestResponse behaviours. Thus
when handling RequestId in the context of multiple
RequestResponse behaviours, one needs to couple the protocol name
with the RequestId to get a unique request identifier.

This commit ensures that pending requests (pending_requests) and
pending responses (pending_response_arrival_time) are tracked both by
their protocol name and RequestId.

Fixes paritytech/polkadot#2307.

`RequestId` is a monotonically increasing integer, starting at
`1`. A `RequestId` is unique for a single `RequestResponse`
behaviour, but not across multiple `RequestResponse` behaviours. Thus
when handling `RequestId` in the context of multiple
`RequestResponse` behaviours, one needs to couple the protocol name
with the `RequestId` to get a unique request identifier.

This commit ensures that pending requests (`pending_requests`) and
pending responses (`pending_response_arrival_time`) are tracked both by
their protocol name and `RequestId`.
@mxinden mxinden added A0-please_review Pull request needs code review. B0-silent Changes should not be mentioned in any release notes C1-low PR touches the given topic and has a low impact on builders. labels Jan 22, 2021
@mxinden mxinden requested review from pepyakin, romanb and tomaka January 22, 2021 17:59
Copy link
Contributor

@romanb romanb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I only left minor comments / suggestions. Overall looks good to me!

Copy link
Contributor

@tomaka tomaka left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Copy link
Contributor

@pepyakin pepyakin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I haven't checked the code, however, I did rerun the test. All 3 validators are running fine on my localnet for a little bit more than one day.

@mxinden
Copy link
Contributor Author

mxinden commented Jan 25, 2021

Thanks everyone for the testing and reviews! Very much appreciated.

Judging by the comments / emojis / resolved conversations all suggestions have been addressed, thus I am merging here. In case there is anything else missing I am happy to create another pull request.

@mxinden
Copy link
Contributor Author

mxinden commented Jan 25, 2021

bot merge

@ghost
Copy link

ghost commented Jan 25, 2021

Trying merge.

@ghost ghost merged commit 38f723b into paritytech:master Jan 25, 2021
This pull request was closed.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

A0-please_review Pull request needs code review. B0-silent Changes should not be mentioned in any release notes C1-low PR touches the given topic and has a low impact on builders.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

libp2p crash related for request-response

5 participants