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

UrlConnection timeout required for FeedEntryMessageSource #3677

Closed
pinkeshsagar-harptec opened this issue Nov 16, 2021 · 8 comments · Fixed by #3678
Closed

UrlConnection timeout required for FeedEntryMessageSource #3677

pinkeshsagar-harptec opened this issue Nov 16, 2021 · 8 comments · Fixed by #3678

Comments

@pinkeshsagar-harptec
Copy link

pinkeshsagar-harptec commented Nov 16, 2021

Expected Behavior

FeedEntryMessageSource should timeout the connection when server is unresponsive for desired amount of time

Current Behavior

FeedEntryMessageSource holds the connection infinitely which is blocking other threads.

Here is stackoverflow url: https://stackoverflow.com/questions/69822045/threads-are-being-blocked-while-receiving-data-from-rss-feed-adapter?noredirect=1#comment123697506_69822045

@pinkeshsagar-harptec pinkeshsagar-harptec added status: waiting-for-triage The issue need to be evaluated and its future decided type: enhancement labels Nov 16, 2021
@artembilan
Copy link
Member

It looks like the best way is to expose an UrlConnection as a FeedEntryMessageSource configuration variant.

See the linked SO thread for a workaround.

Will fix this soon.

@artembilan artembilan removed the status: waiting-for-triage The issue need to be evaluated and its future decided label Nov 16, 2021
@artembilan artembilan added this to the 5.5.6 milestone Nov 16, 2021
@garyrussell
Copy link
Contributor

Actually, I don't think we can do that - the UrlConnection would be opened too soon.

Maybe a Supplier<UrlConnection> ?

@artembilan
Copy link
Member

artembilan commented Nov 16, 2021

Why is that, Gary?
Connection is really done via connect().
Even URL.openConnection() really does not open it, but just create a new instance:

public URLConnection openConnection() throws java.io.IOException {
       return handler.openConnection(this);
}
...
sun.net.www.protocol.http.Handler
...
protected java.net.URLConnection openConnection(URL u, Proxy p)
        throws IOException {
        return new HttpURLConnection(u, p, this);
}

Essentially the same is done in the:

   public XmlReader(final URL url, final Map<String, String> requestHeaders) throws IOException {
        this(url.openConnection(), requestHeaders);
    }

@garyrussell
Copy link
Contributor

Right; but the reader is created at runtime in getFeed() whenever the fetch list is empty - I don't think we can reuse the same connection each time.

@garyrussell
Copy link
Contributor

garyrussell commented Nov 16, 2021

That said, given that the UrlResource allows customization (as you described on Stack Overflow), do we really need this change at all?

@artembilan
Copy link
Member

I don't think we can reuse the same connection each time.

I see your point.

So, yeah... Turning this issue to docs with respective fix in the feed.adoc about possible way to customize.

artembilan added a commit to artembilan/spring-integration that referenced this issue Nov 16, 2021
Fixes spring-projects#3677

If there is need to have a `URLConnection` customized, the `UrlResource`
has to be used instead of plain `URL` injection into the `FeedEntryMessageSource`
garyrussell pushed a commit that referenced this issue Nov 16, 2021
* GH-3677: Doc for URL conn customization in FeedCA

Fixes #3677

If there is need to have a `URLConnection` customized, the `UrlResource`
has to be used instead of plain `URL` injection into the `FeedEntryMessageSource`

* * Add a sample to docs for connection customization
@pinkeshsagar-harptec
Copy link
Author

So does this mean it will be a sequential execution for multiple threads accessing different urls?

@artembilan
Copy link
Member

I’m not sure why make such an assumption, but it probably deserves its own SO thread with much more info

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

Successfully merging a pull request may close this issue.

3 participants