New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix an duplicated key issue #364

Merged
merged 4 commits into from Jun 29, 2018

Conversation

Projects
None yet
3 participants
@Ariex
Copy link
Contributor

Ariex commented Jun 29, 2018

I got a weird issue recently, the code randomly throw an exception which says

The active test run was aborted. Reason: Unhandled Exception: System.ArgumentException: An item with the same key has already been added. Key: https://cdn.jsdelivr.net/npm/font-awesome@4.7.0/fonts/fontawesome-webfont.woff2?v=4.7.0

However I did not able to identify why this is happening in my project, and Fiddler also says there is only one request logged to that url, so I cannot create a demo page for you.

I have similar code running based on GoogleChrome/puppeteer but never got this issue, so I digged a little in its code and found a minor difference at here.

As you can see, the responses is a Map object, and it uses responses.set(response.url(), response) to update it. And in puppeteer-sharp, the corresponding responses object is using a Dictionary to contains this key-value pair.

The difference is, Map.set is capable of 2 things: create the key-value pair when key not exist, and update the value if key exists. But Dictionary.Add in .net will throw exception if key already in dictionary.

So I modified the code to make it update the value if the key already exists.

Thanks.

Bo Zhao

@Ariex Ariex changed the title fix an duplicated key issue, so the code behaviour same as github.com… fix an duplicated key issue Jun 29, 2018

@kblok
Copy link
Owner

kblok left a comment

We could do just
responses[e.Response.Url] = e.Response;

responses.Add(e.Response.Url, e.Response);
{
responses[e.Response.Url] = e.Response;
};

This comment has been minimized.

@kblok

kblok Jun 29, 2018

Owner

@Meir017 would you use brackets here?

This comment has been minimized.

@Ariex

Ariex Jun 29, 2018

Contributor

I will remove it to keep the code style consistent

@Ariex

This comment has been minimized.

Copy link
Contributor

Ariex commented Jun 29, 2018

hi i totally agree with you, i have made another change, thanks a lot.

@@ -1283,8 +1283,7 @@ public Task<ElementHandle> WaitForXPathAsync(string xpath, WaitForSelectorOption
var watcher = new NavigatorWatcher(_frameManager, mainFrame, timeout, options);
var responses = new Dictionary<string, Response>();

EventHandler<ResponseCreatedEventArgs> createResponseEventListener = (object sender, ResponseCreatedEventArgs e) =>
responses.Add(e.Response.Url, e.Response);
EventHandler<ResponseCreatedEventArgs> createResponseEventListener = (object sender, ResponseCreatedEventArgs e) => responses[e.Response.Url] = e.Response;

This comment has been minimized.

@kblok

kblok Jun 29, 2018

Owner

Don't hate me 😂 But this could be shorter like this

EventHandler<ResponseCreatedEventArgs> createResponseEventListener = (sender, e) => responses[e.Response.Url] = e.Response;

This comment has been minimized.

@Ariex

Ariex Jun 29, 2018

Contributor

oh, I did not notice that because that is what the original code is.. I will make another change now, thanks~

@kblok

This comment has been minimized.

Copy link
Owner

kblok commented Jun 29, 2018

Awesome!
Will you need a nuget package update? I can build a 1.1.1 for you.

@Ariex

This comment has been minimized.

Copy link
Contributor

Ariex commented Jun 29, 2018

I am ok, I am currently made a local copy of your code with minor modification then added a local reference to it in my demo project.
I just want to see how this works and if it is good enough to cover my requirements. And so far all good,

Thanks for this excellent project!

@kblok

This comment has been minimized.

Copy link
Owner

kblok commented Jun 29, 2018

Thank you man!

@Ariex

This comment has been minimized.

Copy link
Contributor

Ariex commented Jun 29, 2018

Any idea what is wrong with the ci check? PuppeteerSharp.Tests.PageTests.ScreenshotTests.ShouldRunInParallel is all good in my local test.

Thanks

@kblok

kblok approved these changes Jun 29, 2018

@kblok

This comment has been minimized.

Copy link
Owner

kblok commented Jun 29, 2018

@Ariex we are green, just AppVeyor being AppVeyor 😂

@kblok kblok merged commit 1b97056 into kblok:master Jun 29, 2018

2 checks passed

CodeFactor No issues found.
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment