otiai10 / opengraph

Open Graph Parser for Go
https://pkg.go.dev/github.com/otiai10/opengraph
MIT License
65 stars 13 forks source link

Fetch should take context.Context so that multiple fetching can share their context (e.g. Timeout) #5

Closed kaboc closed 5 years ago

kaboc commented 5 years ago

Timeout and cancellation of a fetch process can be controlled as below.

ctx, _ := context.WithTimeout(context.Background(), 10*time.Second)
og, err := opengraph.FetchWithContext(ctx, "https://github.com/otiai10")

I wanted to get this feature to be contained in the existing Fetch(), but the function structure did not allow it and I did not come up with a better way than adding FetchWithContext().

More importantly, I'm afraid this requires Go 1.7 or higher as WithContext() was introduced in that version.

codecov-io commented 5 years ago

Codecov Report

Merging #5 into master will decrease coverage by 0.66%. The diff coverage is 71.42%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master       #5      +/-   ##
==========================================
- Coverage   83.56%   82.89%   -0.67%     
==========================================
  Files           4        4              
  Lines         146      152       +6     
==========================================
+ Hits          122      126       +4     
- Misses         16       17       +1     
- Partials        8        9       +1
Impacted Files Coverage Δ
opengraph.go 77.21% <71.42%> (-0.87%) :arrow_down:

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update b8b630b...1b4f85b. Read the comment docs.

kaboc commented 5 years ago

@otiai10 Added a test code.

It fetches OG info three times in sequence from a slow server that takes 300 ms to handle every request in the context where timeout is set to be 750 ms. The first one or two complete early and only the last one does not make it before the timeout. Is it sufficient and appropriate?

The known issue in the same file probably need to be fixed before you try running the newly added test.

otiai10 commented 5 years ago

The known issue in the same file probably need to be fixed before you try running the newly added test.

Sorry, could you please give me the specific lines?

kaboc commented 5 years ago

Sorry, could you please give me the specific lines?

I apologize I realized just now that all the tests were successful on Travis-CI. I misunderstood there must be something to be fixed because TestFetch failed in my local environment. Just ignore it if there is no problem in your environment.

kaboc commented 5 years ago

@otiai10 I simplified and improved the test. Now a single context per a pair of one request and one assertion. The first one of two fetches goes successfully and the second one gets cancelled.

otiai10 commented 5 years ago

@kaboc Thank you very much!

Then, I guess Fetch(url, &http.Client{Timeout: 300 * time.Millisecond}) also can pass these test cases.

otiai10 commented 5 years ago

@kaboc I changed the title. Is it surely what you want?

kaboc commented 5 years ago

@otiai10 Yes. It describes my intention more clearly. Thanks!