search

kolo / xmlrpc

Implementation of XMLRPC protocol in Go language.
MIT License
159 stars 94 forks source link

"connection is shut down" issue occured again #63

Closed sonic0002 closed 5 years ago

sonic0002 commented 5 years ago

hello, admin, it seems we see that the same issue occured again with latest codebase, originally you guys have fixed this, but later it seemed you added some code which reintroduced the issue again?

The code which caused this same issue now is the below code block(in client.go) per our understanding .

    if httpResponse.StatusCode < 200 || httpResponse.StatusCode >= 300 {
        return fmt.Errorf("request error: bad status code - %d", httpResponse.StatusCode)
    }

Based on our checking so far, when error returned from above code, the caller for loop breaks and client.shutdown is set to true and all following requests get failed.

Can you confirm this issue and if yes any plan to fix it? Thanks.

@kolo @miracle2k

sonic0002 commented 5 years ago

any update on this? @kolo @miracle2k

icholy commented 5 years ago

Seems like we should be setting rpc.Response.Error instead of returning it.

icholy commented 5 years ago

@kolo does implementing net/rcp.ClientCodec actually buy us anything? It seems like it's forcing us to re-invent logic that http.Client already implements.

sonic0002 commented 5 years ago

@icholy agreed

Seems like we should be setting rpc.Response.Error instead of returning it.

This error would block all the following requests and you have to close the current client and start a new one. Think this is not an ideal solution.

sonic0002 commented 5 years ago

cc @harrycmfan

icholy commented 5 years ago

@sonic0002 can you try the error_handling branch?

kolo commented 5 years ago

@icholy implementing net/rpc.ClientCodec doesn't give much. Initially (7 years ago) I thought it would be nice to implement it but now I don't see any benefit of doing that. It was one of the reasons why proper multicall wasn't implemented for example.

I'm not sure though how Client is used. Does anyone ever use interface over actual type? This is the question without an answer for me.

icholy commented 5 years ago

@kolo I guess writing code that uses rcp.Client lets you swap out codecs without changing your client code. But I've never seen this done in practice.

sonic0002 commented 5 years ago

@icholy are we sure this will solve the problem? Have you tested it and can this be merged to master branch? We cannot test and verify this on our prod env.

icholy commented 5 years ago

@sonic0002 if you're not going to help, please don't add noise.