Skip to content
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

Add url connection and read timeouts #28

Merged
merged 1 commit into from
Nov 10, 2023
Merged

Add url connection and read timeouts #28

merged 1 commit into from
Nov 10, 2023

Conversation

pavlogs
Copy link
Contributor

@pavlogs pavlogs commented Nov 10, 2023

Resolution for Issue #8

@asolntsev asolntsev self-requested a review November 10, 2023 05:49
@asolntsev asolntsev self-assigned this Nov 10, 2023
@asolntsev asolntsev added this to the 1.9.0 milestone Nov 10, 2023
@asolntsev
Copy link
Member

@pavlogs Don't we want to make this timeout configurable? Maybe 10 seconds is not enough in some projects?..

@asolntsev asolntsev merged commit 4991489 into codeborne:main Nov 10, 2023
4 checks passed
@asolntsev asolntsev linked an issue Nov 10, 2023 that may be closed by this pull request
@pavlogs
Copy link
Contributor Author

pavlogs commented Nov 10, 2023

@pavlogs Don't we want to make this timeout configurable? Maybe 10 seconds is not enough in some projects?..

@asolntsev Good point. IMHO a separate configuration class should be introduced to have greater flexibility and handle such cases.
I can open feature request for later. WDYT?

@asolntsev
Copy link
Member

Maybe just add another parameter to the related methods?

Any experience with configuration class shows that such a class can quickly grow into an uncontrollable monster with tons of settings. I am a bit afraid of it.

@pavlogs
Copy link
Contributor Author

pavlogs commented Nov 11, 2023

grow into an uncontrollable monster

Would agree about this statement 🙂

Maybe just add another parameter to the related methods?

That method is private so it won't affect publicly available api. As an option and to be consistent with a current approach, another constructor could be introduced (see example commit). However it will increase complexity of existing constructors (eg. this two).
Seems like usage of builder suites this class. WDYT? I can open another PR if you think such way of configuration is good enough.

@asolntsev
Copy link
Member

This private method is used in exactly two public constructors:

  • new PDF(URL url), and
  • new PDF(URL url, int startPage, int endPage).

I agree, adding 2 more methods would be too much.
Probably yes, the builder is a good idea.

@pavlogs
Copy link
Contributor Author

pavlogs commented Nov 21, 2023

Hello, @asolntsev 👋
Finally got a chance to dig into the timeout setup issue. After looking into it, I think going for a builder or factory method might introduce some breaking changes, and I'd rather avoid that. Plus, you've got another project, xls-test, that would need the same changes to keep consistency.
There is another way how it can be approached - by making readBytes publicly available
Please take a look and if you are ok with this solution I will open a PR

@asolntsev
Copy link
Member

@pavlogs Yes, we can do a similar change in xls-test, why not?

Sorry, I didn't undestand how making readBytes public solves our problem.

@pavlogs
Copy link
Contributor Author

pavlogs commented Nov 25, 2023

Excuse me for any possible confusion. The general idea was to make timeout configurable. I thought it would be clear from the added tests how it could be achieved. From the framework point of view only readBytes with URLConnection in the signature was added. Constructor with byte[] is available.
So, to make long story short. Adding public method is not mandatory - url connection timeout could be configured without changing anything.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Set timeout when reading PDF from URL
2 participants