MagicTheGathering / mtg-sdk-java

Magic: The Gathering SDK - Java
MIT License
67 stars 23 forks source link

Socket timeout exception querying all sets #36

Closed relnah closed 4 years ago

relnah commented 4 years ago

When querying for all sets I get socket timeout exception fairly often. I then do a simple request in browser and get a normal response in around 11s, can't se socket connection.

Would be nice to have a little less trigger happy socket timeout or maybe be able to configure the timeout through the API lib. The mtg REST api will probably be a bit slow from time to time.

thechucklingatom commented 4 years ago

in version 0.0.16 the socket timeout has been updated to 60s, which should resolve this issue.

Doubi88 commented 4 years ago

Hey, sorry for answering to a closed issue, but I still have the same problem with version 0.0.16. It doesn't matter which query I call, I always get a socket timeout exception. As an axample, this is my code for searching a card by name:

    public List<Card> findCards(String name, Optional<String> setCode) {
        ArrayList<String> params = new ArrayList<>(2);
        params.add("name=" + name);
        setCode.ifPresent(str -> params.add("set=" + str));
        return CardAPI.getAllCards(params).stream()
                      .filter(card -> card.getMultiverseid() >= 0)
                      .collect(Collectors.toList());
    }

Du you have any suggestions, where this might come from?

thechucklingatom commented 4 years ago

I do know that the API has a tendency to have latency spikes. There is some thought to allow a custom timeout so the user can set a timeout that is reasonable to them. Unfortunately, I don't have a way of accessing the API itself to give an idea of the latency spikes. Do you always get this issue or just sporadically?

Doubi88 commented 4 years ago

Ok, thanks for the reply. I would rather say, it works sporadically ;-) I tried it a few times last week and it always happened. This morning, it worked and i got results, but now the timeout happens again. Yes, a custom timeout would be a good idea. Do you know of an alredy existing issue discussing the latency of the API-Server? Or anything they could be working on regarding this?

thechucklingatom commented 4 years ago

The API issues page is listed here https://github.com/MagicTheGathering/mtg-api/issues. I do not see any discussion there though. I will start doing some research on the best way to allow a custom timeout. I am a bit slammed right now so it may be a while before a change for that comes in.

Doubi88 commented 4 years ago

Ok thanks. Don't stress yourself ;-)

Doubi88 commented 4 years ago

Something like this?

thechucklingatom commented 4 years ago

That looks perfect, thank you for your contribution. I have merged the pull request and updated the version. Should be accessible in the repositories shortly.

Doubi88 commented 4 years ago

Hey, it's me again. After testing it and further reading in the OkHTTPClient-Docs, I found out why the timeout didn't work for me: We have to set the readTimeout for this. The connectTimeout is for the time it needs to connect to the server, the readTimeout for the time the server needs to send the response.

I can to that this evening if you want. (Sorry, that I didn't check this earlier)

thechucklingatom commented 4 years ago

That makes sense, it may be nice to have both configurable. If you would like to put in the effort to implement that I will review and merge once the pull request comes in.