Make concurrent reads on HttpHeaders thread-safe by MihaZupan · Pull Request #68115 · dotnet/runtime

MihaZupan · 2022-04-16T18:45:51Z

Fixes#61798
Lowers the impact of bugs like #66989 and #65379.

Adds no overhead if headers are both added & enumerated without validation.

Simpler to review when hiding whitespace changes.

ghost · 2022-04-16T18:46:01Z

Tagging subscribers to this area: @dotnet/ncl
See info in area-owners.md if you want to be subscribed.

Issue Details

Fixes #61798

Adds no overhead if headers are both added & enumerated without validation.

Author:	MihaZupan
Assignees:	MihaZupan
Labels:	`area-System.Net.Http`
Milestone:	7.0.0

MihaZupan · 2022-04-21T14:27:04Z

@dotnet/ncl PTAL

The change boils down to adding a lock when:

The values in HeaderStoreItemInfo are being parsed (so its fields are being modified)
The values are read from HeaderStoreItemInfo (non-validated reads / copying to a new header collection).

Since a value entry can transition from being a string to a HeaderStoreItemInfo, and we're not locking at those sites, it is expected that a single raw value may end up being parsed multiple times.
Such parsing is idempotent, so this is transparent to the user.

The code also has to be careful when reading the value of a given entry.
For example, the following logic would be problematic:

if(entries[i].ValueisHeaderStoreItemInfo){// ...}else{DoSomething((string)entries[i].Value);// Wrong! The value may now be HeaderStoreItemInfo}

and it should instead be something like

objectvalue=entries[i].Value;if(valueisHeaderStoreItemInfo){// ...}else{DoSomething((string)value);}

I've looked through our current code to make sure this is the case, but it's possible future changes may accidentally break this. I've added a test that should catch most such mistakes, but there may be cases that aren't covered.

CarnaViire

LGTM, but maybe someone else could also take a look 🙂

CarnaViire · 2022-04-29T11:19:36Z

Does this change fully address #61798 ? I see the issue mentioning invalid values too, but this change only adds locks?

MihaZupan · 2022-04-29T11:35:22Z

Does this change fully address #61798 ?

Yes. I split off the invalid values part into #67199, as that was a prerequisite for this change.

juharris · 2022-05-09T20:08:07Z

Will this change be in an upcoming version 6 update? If so, any idea when?

MihaZupan · 2022-05-09T21:15:35Z

It will be in preview 5 of .NET 7.

If you mean whether it will be backported to 6.0, there are currently no plans for that.

MihaZupan · 2022-04-16T18:45:51Z

Fixes#61798
Lowers the impact of bugs like #66989 and #65379.

Adds no overhead if headers are both added & enumerated without validation.

Simpler to review when hiding whitespace changes.

ghost · 2022-04-16T18:46:01Z

Tagging subscribers to this area: @dotnet/ncl
See info in area-owners.md if you want to be subscribed.

Issue Details

Fixes #61798

Adds no overhead if headers are both added & enumerated without validation.

Author:	MihaZupan
Assignees:	MihaZupan
Labels:	`area-System.Net.Http`
Milestone:	7.0.0

MihaZupan · 2022-04-21T14:27:04Z

@dotnet/ncl PTAL

The change boils down to adding a lock when:

The values in HeaderStoreItemInfo are being parsed (so its fields are being modified)
The values are read from HeaderStoreItemInfo (non-validated reads / copying to a new header collection).

Since a value entry can transition from being a string to a HeaderStoreItemInfo, and we're not locking at those sites, it is expected that a single raw value may end up being parsed multiple times.
Such parsing is idempotent, so this is transparent to the user.

The code also has to be careful when reading the value of a given entry.
For example, the following logic would be problematic:

if(entries[i].ValueisHeaderStoreItemInfo){// ...}else{DoSomething((string)entries[i].Value);// Wrong! The value may now be HeaderStoreItemInfo}

and it should instead be something like

objectvalue=entries[i].Value;if(valueisHeaderStoreItemInfo){// ...}else{DoSomething((string)value);}

I've looked through our current code to make sure this is the case, but it's possible future changes may accidentally break this. I've added a test that should catch most such mistakes, but there may be cases that aren't covered.

CarnaViire

LGTM, but maybe someone else could also take a look 🙂

CarnaViire · 2022-04-29T11:19:36Z

Does this change fully address #61798 ? I see the issue mentioning invalid values too, but this change only adds locks?

MihaZupan · 2022-04-29T11:35:22Z

Does this change fully address #61798 ?

Yes. I split off the invalid values part into #67199, as that was a prerequisite for this change.

juharris · 2022-05-09T20:08:07Z

Will this change be in an upcoming version 6 update? If so, any idea when?

MihaZupan · 2022-05-09T21:15:35Z

It will be in preview 5 of .NET 7.

If you mean whether it will be backported to 6.0, there are currently no plans for that.

MihaZupan · 2022-04-16T18:45:51Z

Fixes#61798
Lowers the impact of bugs like #66989 and #65379.

Adds no overhead if headers are both added & enumerated without validation.

Simpler to review when hiding whitespace changes.

ghost · 2022-04-16T18:46:01Z

Tagging subscribers to this area: @dotnet/ncl
See info in area-owners.md if you want to be subscribed.

Issue Details

Fixes #61798

Adds no overhead if headers are both added & enumerated without validation.

Author:	MihaZupan
Assignees:	MihaZupan
Labels:	`area-System.Net.Http`
Milestone:	7.0.0

MihaZupan · 2022-04-21T14:27:04Z

@dotnet/ncl PTAL

The change boils down to adding a lock when:

The values in HeaderStoreItemInfo are being parsed (so its fields are being modified)
The values are read from HeaderStoreItemInfo (non-validated reads / copying to a new header collection).

Since a value entry can transition from being a string to a HeaderStoreItemInfo, and we're not locking at those sites, it is expected that a single raw value may end up being parsed multiple times.
Such parsing is idempotent, so this is transparent to the user.

The code also has to be careful when reading the value of a given entry.
For example, the following logic would be problematic:

if(entries[i].ValueisHeaderStoreItemInfo){// ...}else{DoSomething((string)entries[i].Value);// Wrong! The value may now be HeaderStoreItemInfo}

and it should instead be something like

objectvalue=entries[i].Value;if(valueisHeaderStoreItemInfo){// ...}else{DoSomething((string)value);}

I've looked through our current code to make sure this is the case, but it's possible future changes may accidentally break this. I've added a test that should catch most such mistakes, but there may be cases that aren't covered.

CarnaViire

LGTM, but maybe someone else could also take a look 🙂

CarnaViire · 2022-04-29T11:19:36Z

Does this change fully address #61798 ? I see the issue mentioning invalid values too, but this change only adds locks?

MihaZupan · 2022-04-29T11:35:22Z

Does this change fully address #61798 ?

Yes. I split off the invalid values part into #67199, as that was a prerequisite for this change.

juharris · 2022-05-09T20:08:07Z

Will this change be in an upcoming version 6 update? If so, any idea when?

MihaZupan · 2022-05-09T21:15:35Z

It will be in preview 5 of .NET 7.

If you mean whether it will be backported to 6.0, there are currently no plans for that.

MihaZupan · 2022-04-16T18:45:51Z

Fixes#61798
Lowers the impact of bugs like #66989 and #65379.

Adds no overhead if headers are both added & enumerated without validation.

Simpler to review when hiding whitespace changes.

ghost · 2022-04-16T18:46:01Z

Tagging subscribers to this area: @dotnet/ncl
See info in area-owners.md if you want to be subscribed.

Issue Details

Fixes #61798

Adds no overhead if headers are both added & enumerated without validation.

Author:	MihaZupan
Assignees:	MihaZupan
Labels:	`area-System.Net.Http`
Milestone:	7.0.0

MihaZupan · 2022-04-21T14:27:04Z

@dotnet/ncl PTAL

The change boils down to adding a lock when:

The values in HeaderStoreItemInfo are being parsed (so its fields are being modified)
The values are read from HeaderStoreItemInfo (non-validated reads / copying to a new header collection).

Since a value entry can transition from being a string to a HeaderStoreItemInfo, and we're not locking at those sites, it is expected that a single raw value may end up being parsed multiple times.
Such parsing is idempotent, so this is transparent to the user.

The code also has to be careful when reading the value of a given entry.
For example, the following logic would be problematic:

if(entries[i].ValueisHeaderStoreItemInfo){// ...}else{DoSomething((string)entries[i].Value);// Wrong! The value may now be HeaderStoreItemInfo}

and it should instead be something like

objectvalue=entries[i].Value;if(valueisHeaderStoreItemInfo){// ...}else{DoSomething((string)value);}

I've looked through our current code to make sure this is the case, but it's possible future changes may accidentally break this. I've added a test that should catch most such mistakes, but there may be cases that aren't covered.

CarnaViire

LGTM, but maybe someone else could also take a look 🙂

CarnaViire · 2022-04-29T11:19:36Z

Does this change fully address #61798 ? I see the issue mentioning invalid values too, but this change only adds locks?

MihaZupan · 2022-04-29T11:35:22Z

Does this change fully address #61798 ?

Yes. I split off the invalid values part into #67199, as that was a prerequisite for this change.

juharris · 2022-05-09T20:08:07Z

Will this change be in an upcoming version 6 update? If so, any idea when?

MihaZupan · 2022-05-09T21:15:35Z

It will be in preview 5 of .NET 7.

If you mean whether it will be backported to 6.0, there are currently no plans for that.

MihaZupan · 2022-04-16T18:45:51Z

Fixes#61798
Lowers the impact of bugs like #66989 and #65379.

Adds no overhead if headers are both added & enumerated without validation.

Simpler to review when hiding whitespace changes.

ghost · 2022-04-16T18:46:01Z

Tagging subscribers to this area: @dotnet/ncl
See info in area-owners.md if you want to be subscribed.

Issue Details

Fixes #61798

Adds no overhead if headers are both added & enumerated without validation.

Author:	MihaZupan
Assignees:	MihaZupan
Labels:	`area-System.Net.Http`
Milestone:	7.0.0

MihaZupan · 2022-04-21T14:27:04Z

@dotnet/ncl PTAL

The change boils down to adding a lock when:

The values in HeaderStoreItemInfo are being parsed (so its fields are being modified)
The values are read from HeaderStoreItemInfo (non-validated reads / copying to a new header collection).

Since a value entry can transition from being a string to a HeaderStoreItemInfo, and we're not locking at those sites, it is expected that a single raw value may end up being parsed multiple times.
Such parsing is idempotent, so this is transparent to the user.

The code also has to be careful when reading the value of a given entry.
For example, the following logic would be problematic:

if(entries[i].ValueisHeaderStoreItemInfo){// ...}else{DoSomething((string)entries[i].Value);// Wrong! The value may now be HeaderStoreItemInfo}

and it should instead be something like

objectvalue=entries[i].Value;if(valueisHeaderStoreItemInfo){// ...}else{DoSomething((string)value);}

I've looked through our current code to make sure this is the case, but it's possible future changes may accidentally break this. I've added a test that should catch most such mistakes, but there may be cases that aren't covered.

CarnaViire

LGTM, but maybe someone else could also take a look 🙂

CarnaViire · 2022-04-29T11:19:36Z

Does this change fully address #61798 ? I see the issue mentioning invalid values too, but this change only adds locks?

MihaZupan · 2022-04-29T11:35:22Z

Does this change fully address #61798 ?

Yes. I split off the invalid values part into #67199, as that was a prerequisite for this change.

juharris · 2022-05-09T20:08:07Z

Will this change be in an upcoming version 6 update? If so, any idea when?

MihaZupan · 2022-05-09T21:15:35Z

It will be in preview 5 of .NET 7.

If you mean whether it will be backported to 6.0, there are currently no plans for that.

MihaZupan · 2022-04-16T18:45:51Z

Fixes#61798
Lowers the impact of bugs like #66989 and #65379.

Adds no overhead if headers are both added & enumerated without validation.

Simpler to review when hiding whitespace changes.

ghost · 2022-04-16T18:46:01Z

Tagging subscribers to this area: @dotnet/ncl
See info in area-owners.md if you want to be subscribed.

Issue Details

Fixes #61798

Adds no overhead if headers are both added & enumerated without validation.

Author:	MihaZupan
Assignees:	MihaZupan
Labels:	`area-System.Net.Http`
Milestone:	7.0.0

MihaZupan · 2022-04-21T14:27:04Z

@dotnet/ncl PTAL

The change boils down to adding a lock when:

The values in HeaderStoreItemInfo are being parsed (so its fields are being modified)
The values are read from HeaderStoreItemInfo (non-validated reads / copying to a new header collection).

Since a value entry can transition from being a string to a HeaderStoreItemInfo, and we're not locking at those sites, it is expected that a single raw value may end up being parsed multiple times.
Such parsing is idempotent, so this is transparent to the user.

The code also has to be careful when reading the value of a given entry.
For example, the following logic would be problematic:

if(entries[i].ValueisHeaderStoreItemInfo){// ...}else{DoSomething((string)entries[i].Value);// Wrong! The value may now be HeaderStoreItemInfo}

and it should instead be something like

objectvalue=entries[i].Value;if(valueisHeaderStoreItemInfo){// ...}else{DoSomething((string)value);}

I've looked through our current code to make sure this is the case, but it's possible future changes may accidentally break this. I've added a test that should catch most such mistakes, but there may be cases that aren't covered.

CarnaViire

LGTM, but maybe someone else could also take a look 🙂

CarnaViire · 2022-04-29T11:19:36Z

Does this change fully address #61798 ? I see the issue mentioning invalid values too, but this change only adds locks?

MihaZupan · 2022-04-29T11:35:22Z

Does this change fully address #61798 ?

Yes. I split off the invalid values part into #67199, as that was a prerequisite for this change.

juharris · 2022-05-09T20:08:07Z

Will this change be in an upcoming version 6 update? If so, any idea when?

MihaZupan · 2022-05-09T21:15:35Z

It will be in preview 5 of .NET 7.

If you mean whether it will be backported to 6.0, there are currently no plans for that.

MihaZupan · 2022-04-16T18:45:51Z

Fixes#61798
Lowers the impact of bugs like #66989 and #65379.

Adds no overhead if headers are both added & enumerated without validation.

Simpler to review when hiding whitespace changes.

ghost · 2022-04-16T18:46:01Z

Tagging subscribers to this area: @dotnet/ncl
See info in area-owners.md if you want to be subscribed.

Issue Details

Fixes #61798

Adds no overhead if headers are both added & enumerated without validation.

Author:	MihaZupan
Assignees:	MihaZupan
Labels:	`area-System.Net.Http`
Milestone:	7.0.0

MihaZupan · 2022-04-21T14:27:04Z

@dotnet/ncl PTAL

The change boils down to adding a lock when:

The values in HeaderStoreItemInfo are being parsed (so its fields are being modified)
The values are read from HeaderStoreItemInfo (non-validated reads / copying to a new header collection).

Since a value entry can transition from being a string to a HeaderStoreItemInfo, and we're not locking at those sites, it is expected that a single raw value may end up being parsed multiple times.
Such parsing is idempotent, so this is transparent to the user.

The code also has to be careful when reading the value of a given entry.
For example, the following logic would be problematic:

if(entries[i].ValueisHeaderStoreItemInfo){// ...}else{DoSomething((string)entries[i].Value);// Wrong! The value may now be HeaderStoreItemInfo}

and it should instead be something like

objectvalue=entries[i].Value;if(valueisHeaderStoreItemInfo){// ...}else{DoSomething((string)value);}

I've looked through our current code to make sure this is the case, but it's possible future changes may accidentally break this. I've added a test that should catch most such mistakes, but there may be cases that aren't covered.

CarnaViire

LGTM, but maybe someone else could also take a look 🙂

CarnaViire · 2022-04-29T11:19:36Z

Does this change fully address #61798 ? I see the issue mentioning invalid values too, but this change only adds locks?

MihaZupan · 2022-04-29T11:35:22Z

Does this change fully address #61798 ?

Yes. I split off the invalid values part into #67199, as that was a prerequisite for this change.

juharris · 2022-05-09T20:08:07Z

Will this change be in an upcoming version 6 update? If so, any idea when?

MihaZupan · 2022-05-09T21:15:35Z

It will be in preview 5 of .NET 7.

If you mean whether it will be backported to 6.0, there are currently no plans for that.

MihaZupan · 2022-04-16T18:45:51Z

Fixes#61798
Lowers the impact of bugs like #66989 and #65379.

Adds no overhead if headers are both added & enumerated without validation.

Simpler to review when hiding whitespace changes.

ghost · 2022-04-16T18:46:01Z

Tagging subscribers to this area: @dotnet/ncl
See info in area-owners.md if you want to be subscribed.

Issue Details

Fixes #61798

Adds no overhead if headers are both added & enumerated without validation.

Author:	MihaZupan
Assignees:	MihaZupan
Labels:	`area-System.Net.Http`
Milestone:	7.0.0

MihaZupan · 2022-04-21T14:27:04Z

@dotnet/ncl PTAL

The change boils down to adding a lock when:

The values in HeaderStoreItemInfo are being parsed (so its fields are being modified)
The values are read from HeaderStoreItemInfo (non-validated reads / copying to a new header collection).

Since a value entry can transition from being a string to a HeaderStoreItemInfo, and we're not locking at those sites, it is expected that a single raw value may end up being parsed multiple times.
Such parsing is idempotent, so this is transparent to the user.

The code also has to be careful when reading the value of a given entry.
For example, the following logic would be problematic:

if(entries[i].ValueisHeaderStoreItemInfo){// ...}else{DoSomething((string)entries[i].Value);// Wrong! The value may now be HeaderStoreItemInfo}

and it should instead be something like

objectvalue=entries[i].Value;if(valueisHeaderStoreItemInfo){// ...}else{DoSomething((string)value);}

I've looked through our current code to make sure this is the case, but it's possible future changes may accidentally break this. I've added a test that should catch most such mistakes, but there may be cases that aren't covered.

CarnaViire

LGTM, but maybe someone else could also take a look 🙂

CarnaViire · 2022-04-29T11:19:36Z

Does this change fully address #61798 ? I see the issue mentioning invalid values too, but this change only adds locks?

MihaZupan · 2022-04-29T11:35:22Z

Does this change fully address #61798 ?

Yes. I split off the invalid values part into #67199, as that was a prerequisite for this change.

juharris · 2022-05-09T20:08:07Z

Will this change be in an upcoming version 6 update? If so, any idea when?

MihaZupan · 2022-05-09T21:15:35Z

It will be in preview 5 of .NET 7.

If you mean whether it will be backported to 6.0, there are currently no plans for that.

Make concurrent reads on HttpHeaders thread-safe
29f16fe

MihaZupan added this to the 7.0.0 milestone Apr 16, 2022

MihaZupan self-assigned this Apr 16, 2022

ghost added the area-System.Net.Http label Apr 16, 2022

MihaZupan requested a review from a teamApril 16, 2022 18:47

CarnaViire mentioned this pull request Apr 28, 2022
[API Proposal] ClientWebSocket upgrade response details #25918
Closed

CarnaViire approved these changes Apr 29, 2022
View reviewed changes

Comment threadsrc/libraries/System.Net.Http/tests/UnitTests/Headers/HttpHeadersTest.cs
Comment threadsrc/libraries/System.Net.Http/src/System/Net/Http/Headers/HttpHeaders.cs

ManickaP approved these changes May 6, 2022
View reviewed changes

stephentoub approved these changes May 6, 2022
View reviewed changes

MihaZupan merged commit b00a64d into dotnet:mainMay 6, 2022

LucVL mentioned this pull request May 12, 2022
AmazonS3Client.GetObjectAsync is not threadsafe aws/aws-sdk-net#1971
Closed

ghost locked as resolved and limited conversation to collaborators Jun 9, 2022