Added cookie accessor/iterator to CookieContainer

[shravan2x]

Background and Motivation

There's currently no way to access all cookies in a CookieContainer instance without known their domain names. CookieContainer also does not expose a list of domains it holds cookies for. I propose adding a GetAllCookies() method for this. It seems I'm not the only one with a use-case for it https://stackoverflow.com/questions/15983166/how-can-i-get-all-cookies-of-a-cookiecontainer

Latest API proposal

Based on discussion below - #44094 (comment)

public class CookieContainer
{
  // Existing API:
  public CookieCollection GetCookies(Uri uri);

  // Proposed API:
  public CookieCollection GetAllCookies();
}

Original API proposal

 public IEnumerable<Cookie> GetAllCookies();

or

class CookieContainer : IEnumerable<Cookie>

[Dotnet-GitSync-Bot]

I couldn't figure out the best area label to add to this issue. If you have write-permissions please help me learn by adding exactly one area label.

[ghost]

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

[KalleOlaviNiemitalo]

CookieContainer tries to be thread-safe, although I don't see that in the documentation. If other threads add or remove cookies while you enumerate them, does that invalidate the enumerator?

[shravan2x]

CookieContainer tries to be thread-safe, although I don't see that in the documentation. If other threads add or remove cookies while you enumerate them, does that invalidate the enumerator?

It could invalidate it and throw an exception on the next iteration, or it could iterate an older "snapshot" similar to how ConcurrentDictionary works. Either would make sense to me though the latter would be preferred.

The only bad version would be one that doesn't make any guarantees and just silently corrupts itself to miss entries or return duplicate ones.

[geoffkizer]

Seems like just implementing IEnumerable<Cookie> is the cleanest approach here, unless there's some problem with that.

[geoffkizer]

Actually, we already did this. See dotnet/corefx#30080. I knew it sounded familiar.

[shravan2x]

@geoffkizer That's weird, I just made a new project using .NET 5.0 and I still can't get it to work:

Maybe it was changed after? I don't see it in master either https://github.com/dotnet/runtime/blob/master/src/libraries/System.Net.Primitives/src/System/Net/CookieContainer.cs#L94

[geoffkizer]

Sorry, the linked issue is about CookieCollection, not CookieContainer.

[shravan2x]

Ah that makes sense. Agree that implementing IEnumerable<Cookie> is the cleanest approach.

[KalleOlaviNiemitalo]

Could even implement IReadOnlyCollection<Cookie>. It would add only a Count property to IEnumerable<Cookie>, and CookieContainer.Count exists already.

Implementing IReadOnlyCollection<Cookie> could cause ImmutableArray.CreateRange<T>(IEnumerable<T>) to throw if it first reads Count and allocates an array and then another thread adds more cookies. However, I suppose ConcurrentDictionary<TKey,TValue> already has that problem, so developers can be expected to cope.

Perhaps, instead of having CookieContainer.GetEnumerator make a snapshot, there should be a thread-safe Clone method or a copy constructor. That way, applications that can ensure single-threaded access to CookieContainer would not have to pay for a snapshot, while other applications would be able to clone the container and then enumerate the clone at their leisure.

[geoffkizer]

Yeah, the more I think about this, the more I'm kinda concerned about enumerating a CookieContainer directly.

Another suggestion would be to add a method that creates a snapshot as a CookieCollection, which you can then iterate as you want without worrying about someone else mutating it.

Edit to add:

We already have a GetCookies(uri) method which returns a CookieCollection for a specific uri: https://docs.microsoft.com/en-us/dotnet/api/system.net.cookiecontainer.getcookies?view=net-5.0#System_Net_CookieContainer_GetCookies_System_Uri_

So adding something like GetAllCookies() that returns a CookieCollection for all uris would fit nicely with this.

[shravan2x]

Couldn't we do that (i.e. create a snapshot) internally and iterate over that one in GetEnumerator? It would be the same as you mentioned but avoids the GetAllCookies() extra method.

[geoffkizer]

Why is that better?

[shravan2x]

I suppose it's a matter of opinion, and I hold mine lightly - Since it's a container object (like lists or sets), it should be iterable using a for-each loop or LINQ query.

What are your thoughts on doing both i.e. GetAllCookies() and a new shortcut GetEnumerator() { return Snapshot().GetAllCookies() }?

[geoffkizer]

I think that since we have GetCookies(uri) already, adding something like GetAllCookies() seems pretty natural.

Then the question is, should we also add an enumerator that calls GetAllCookies under the covers? It doesn't add any new capability, and it might not be obvious to users that it implicitly does a snapshot. So I think I'd lean toward not doing this.

Since it's a container object (like lists or sets), it should be iterable using a for-each loop or LINQ query.

I'm somewhat sympathetic to this, but I think this is a bit of an unusual case. CookieCollection is the collection class for Cookies and supports the methods you'd expect -- IEnumerable, ICollection, etc. CookieContainer is optimized for multithreaded access when you are adding/updating/etc cookies. As long as it's easy to go from one to another, I think that's sufficient.

[shravan2x]

Sounds reasonable to me, let's go with GetAllCookies() 👍

[KalleOlaviNiemitalo]

Huh, CookieCollection has a System.Net.Cookie this[string name] indexer, but CookieCollection.Add allows multiple cookies with the same Name to be added to the collection, provided that the Domain or Path differs. I suppose CookieContainer.GetAllCookies() can return a CookieCollection, then.

[geoffkizer]

cc @antonfirsov -- you know this code better than I do, any thoughts?

[antonfirsov]

[...] I suppose CookieContainer.GetAllCookies() can return a CookieCollection, then.

@KalleOlaviNiemitalo considering the first part of the sentence, I guess you rather wanted to say " I suppose CookieContainer.GetAllCookies() can return a CookieCollection, then." :)

So I think I'd lean toward not doing this.

@geoffkizer I agree. Adding a single enumerator method seems to be the safest thing to do here. I don't think we should return CookieCollection. I'm not very familiar enough with call sites, so I may be wrong, but it looks like we rarely use it (0 usages in System.Net.Http, 1 in System.Net.WebSockets, the rest is in System.Net.Requests stuff). Considering the concern pointed out by @KalleOlaviNiemitalo looks like it's technically not possible.

The only remaining open question before an actual API proposal is whether we should return an IReadonlyList<Cookie> or simply just IEnumerable<Cookie>?

[shravan2x]

The only remaining open question before an actual API proposal is whether we should return an IReadonlyList or simply just IEnumerable?

Of course IReadonlyList<Cookie> has slightly more methods, but IEnumerable<Cookie> is probably easier to implement and solves all for-each and LINQ usages. People could also just call .ToList() trivially. So I think IEnumerable<Cookie> is a good option.

[KalleOlaviNiemitalo]

@antonfirsov, isn't that exactly what I wrote already?

To elaborate: The CookieCollection.Item[string name] indexer gives the impression that CookieCollection (source) has at most one cookie for each name. CookieCollection.Add(Cookie) however has no such restriction: it calls CookieCollection.IndexOf(Cookie), which uses CookieComparer (source), which compares Name, Domain, and Path. Thus, it would be technically possible to have CookieContainer.GetAllCookies() return a CookieCollection, even if that might look a bit odd.

[aepot]

I have to bump this issue while a lot of developers still using either Reflection or BinaryFormatter to get all Cookies e.g. to save ones to a file.

As for me it's doesn't matter how accessing to all Cookies will be implemented. I prefer any way to get all Cookies because it's better than no way.

Just need something better than this extension:

public static CookieCollection GetAllCookies(this CookieContainer container)
{
    CookieCollection allCookies = new CookieCollection();
    Hashtable domainTable = (Hashtable)container.GetType()
        .GetRuntimeFields()
        .First(x => x.Name == "m_domainTable")
        .GetValue(container);
    FieldInfo pathListField = null;
    foreach (object domain in domainTable.Values)
    {
        SortedList pathList = (SortedList)(pathListField ??= domain.GetType()
            .GetRuntimeFields()
            .First(x => x.Name == "m_list"))
            .GetValue(domain);
        foreach (CookieCollection cookies in pathList.GetValueList())
            allCookies.Add(cookies);
    }
    return allCookies;
}

The job is loading+saving all the Cookies for HttpClientHandler at the application startup and shutdown. BinaryFormatter helped me before but you know the story.

Also the most popular answers to this problem on StackOverflow have ~77k views in total: 1, 2.

[geoffkizer]

I don't think we have consensus about the API here.

[aepot]

@shravan2x Upon thread-safe CookieContainer nature I suggest a method that returns fullfilled collection (snapshot) as CookieCollection instead of yielding through IEnumerable.

Suggestion1. Add method GetAllCookies()

public CookieCollection GetAllCookies();

Suggestion2. Add overload for GetCookies() that accepts 0 arguments.

public CookieCollection GetCookies();

I think GetAllCookies() looks more explicit as method returning all the cookies. But GetCookies() overload for me looks reasonable too as it was a first thing I tried to get all cookies.

[antonfirsov]

@geoffkizer to me the only open question was if GetAllCookies() should return CookieContainer or just IEnumerable<Cookie>. I was against CookieCollection since it looks like an ugly legacy type, but it's more consistent with the existing CookieContainer API surface so I recommend to go with the following explicit overload:

public CookieCollection GetAllCookies();

If you agree, I recommend to push this into the API review queue, so we have a chance to get this into 6.0. It's a trivially small feature, and it's no fun that reflection+BinaryFormatter (🤯 ) workarounds keep spreading over stackoverflow as described in #44094 (comment). The number of upvotes on SO indicate that this is a visible API gap.

[shravan2x]

@antonfirsov I agree that getting some API out for this functionality is more important than small differences in proposed APIs. At its core CookieContainer seems very similar to ConcurrentDictionary so an iterator could be reconsidered too. I don't have a strong opinion.

@aepot I prefer GetAllCookies() to a GetCookies() overload.

[aepot]

@shravan2x I guess you may edit the first post according to template linked in the API Review Process.

[geoffkizer]

Ok, I updated the initial post to reflect the consensus proposal and marked this as ready for review.

Any feedback is welcome.

[karelz]

@geoffkizer FYI: I updated top post to keep visible Original and Latest API proposals -- easier for following discussions ...

[geoffkizer]

I marked this as 6.0 so it will get reviewed in the near future. That doesn't mean this is committed for 6.0.

[bartonjs]

Video

Looks good as proposed

namespace System.Net
{
    partial class CookieContainer
    {
        public CookieCollection GetAllCookies();
    }
}