Bugzilla – Full Text Bug Listing
| Summary: | [ecf.generic] TCPClientSOContainer has wrong usage of keepAlive timeout | ||||||
|---|---|---|---|---|---|---|---|
| Product: | [RT] ECF | Reporter: | Franky Bridelance <franky.bugzilla> | ||||
| Component: | ecf.providers | Assignee: | Scott Lewis <slewis> | ||||
| Status: | RESOLVED FIXED | QA Contact: | |||||
| Severity: | minor | ||||||
| Priority: | P3 | CC: | slewis | ||||
| Version: | unspecified | ||||||
| Target Milestone: | 3.5.3 | ||||||
| Hardware: | PC | ||||||
| OS: | Windows 7 | ||||||
| Whiteboard: | |||||||
| Attachments: |
|
||||||
|
Description
Franky Bridelance
(In reply to comment #0) > Build Identifier: ECF 3.5.2 > > While it's possible to provide a keepAlive timeout at creation of ecf generic > client container (TCPClientSOContainer), this keepAlive timeout is not used > because the method getConnectTimeout always returns the default timeout (which > is 30 seconds). This means the keepAlive timeout is not configurable. > Moreover the keepAlive timeout is passed as argument to construct a Client > object (in createConnection method) while the argument in Client constructor > represents "max messages" (i.e. number of messages sent on outputStream before > a reset is done). So if you set the keepAlive timeout on TCPClientSOContainer > to 60 seconds (60000 ms) a reset on outputStream in Client object only happens > after 60000 sent messages... I got a out of memory because of this. > I had to subclass TCPClientSOContainer and override getConnectTimeout and > createConnection to make it work correctly but it might be better to solve this > in TCPClientSOContainer as I guess other users can have the same problem. > > Reproducible: Always Hi Franky... So...the original intention for the keepAlive (on the Client) was for the keepAlive interval...so I've fixed the Client/2 constructor to be: public Client(ISynchAsynchEventHandler handler, int keepAlive) { if (handler == null) throw new NullPointerException("event handler cannot be null"); //$NON-NLS-1$ this.handler = handler; this.keepAlive = keepAlive; containerID = handler.getEventHandlerID(); this.properties = new HashMap(); } This should fix the issue you describe here > Moreover the keepAlive timeout is passed as argument to construct a Client > object (in createConnection method) while the argument in Client constructor > represents "max messages" (i.e. number of messages sent on outputStream before > a reset is done). So if you set the keepAlive timeout on TCPClientSOContainer > to 60 seconds (60000 ms) a reset on outputStream in Client object only happens > after 60000 sent messages... I got a out of memory because of this. > I had to subclass TCPClientSOContainer and override getConnectTimeout and > createConnection to make it work correctly but it might be better to solve this > in TCPClientSOContainer as I guess other users can have the same problem. > > Reproducible: Always Now...WRT the first issue...the connect timeout probably should be another/new parameter...and when we go through an API change (addition) I can add this...but in immediate term the only way to configure for the moment is what you did (override getConnectTimeout()). I understand that this is not optimal...but I think perhaps another bug should be opened specifically for this issue (connect timeout configuration) and it should be dealt with through an API change/addition (a connectTimeout parameter). So...if you agree with the change to Client/2 as described above, I'll release it to master, and then we can open another bug for the connect timeout issue...and deal with it during a major version change for o.e.e.provider. Thanks for reporting these issues. (In reply to comment #1) > (In reply to comment #0) > > Build Identifier: ECF 3.5.2 > > > > While it's possible to provide a keepAlive timeout at creation of ecf generic > > client container (TCPClientSOContainer), this keepAlive timeout is not used > > because the method getConnectTimeout always returns the default timeout (which > > is 30 seconds). This means the keepAlive timeout is not configurable. > > Moreover the keepAlive timeout is passed as argument to construct a Client > > object (in createConnection method) while the argument in Client constructor > > represents "max messages" (i.e. number of messages sent on outputStream before > > a reset is done). So if you set the keepAlive timeout on TCPClientSOContainer > > to 60 seconds (60000 ms) a reset on outputStream in Client object only happens > > after 60000 sent messages... I got a out of memory because of this. > > I had to subclass TCPClientSOContainer and override getConnectTimeout and > > createConnection to make it work correctly but it might be better to solve this > > in TCPClientSOContainer as I guess other users can have the same problem. > > > > Reproducible: Always > > Hi Franky... > > So...the original intention for the keepAlive (on the Client) was for the > keepAlive interval...so I've fixed the Client/2 constructor to be: > > public Client(ISynchAsynchEventHandler handler, int keepAlive) { > if (handler == null) > throw new NullPointerException("event handler cannot be null"); > //$NON-NLS-1$ > this.handler = handler; > this.keepAlive = keepAlive; > containerID = handler.getEventHandlerID(); > this.properties = new HashMap(); > } > > This should fix the issue you describe here > > > Moreover the keepAlive timeout is passed as argument to construct a Client > > object (in createConnection method) while the argument in Client constructor > > represents "max messages" (i.e. number of messages sent on outputStream before > > a reset is done). So if you set the keepAlive timeout on TCPClientSOContainer > > to 60 seconds (60000 ms) a reset on outputStream in Client object only happens > > after 60000 sent messages... I got a out of memory because of this. > > I had to subclass TCPClientSOContainer and override getConnectTimeout and > > createConnection to make it work correctly but it might be better to solve this > > in TCPClientSOContainer as I guess other users can have the same problem. > > > > Reproducible: Always > > Now...WRT the first issue...the connect timeout probably should be another/new > parameter...and when we go through an API change (addition) I can add > this...but in immediate term the only way to configure for the moment is what > you did (override getConnectTimeout()). I understand that this is not > optimal...but I think perhaps another bug should be opened specifically for > this issue (connect timeout configuration) and it should be dealt with through > an API change/addition (a connectTimeout parameter). > > So...if you agree with the change to Client/2 as described above, I'll release > it to master, and then we can open another bug for the connect timeout > issue...and deal with it during a major version change for o.e.e.provider. > > Thanks for reporting these issues. Hi Scott, Thanks for the quick response. I'm ok with the Client/2 fix. Concerning the connect timeout: problem is that this connect timeout is at the end used to override the keepAlive of Client during the connect call. So it seems to me that the connect timeout and keepAlive are interchangeable. Let me explain in more detail: After creating a TCPClientSOContainer instance, we need to call connect method to connect to a given server (identified in targetID). This connect method is defined in ClientSOContainer and will: - call createConnection (method of TCPClientSOContainer) which returns a Client instance - call getConnectTimeout (method of TCPClientSOContainer) to get the connectTimeout --> the default 30 sec - call connect method on the connection (Client instance) and pass connectTimeout to this method - in the connect method of Client instance the passed connectTimeout is used to override the keepAlive So at the end no matter what value you give for keepAlive it always ends with the default 30 seconds connect timeout defined in TCPClientSOContainer. It's important in my case to have keepAlive in Client configurable, so either connectTimeout on TCPClientSOContainer must be configurable (if keepAlive and connectTimeout represent the same timeout) or connectTimeout should not be used to override keepAlive in Client connect method (if they don't represent the same timeout). Best regards, Franky (In reply to comment #2) > (In reply to comment #1) > > (In reply to comment #0) > > > Build Identifier: ECF 3.5.2 > > > > > > While it's possible to provide a keepAlive timeout at creation of ecf generic > > > client container (TCPClientSOContainer), this keepAlive timeout is not used > > > because the method getConnectTimeout always returns the default timeout (which > > > is 30 seconds). This means the keepAlive timeout is not configurable. > > > Moreover the keepAlive timeout is passed as argument to construct a Client > > > object (in createConnection method) while the argument in Client constructor > > > represents "max messages" (i.e. number of messages sent on outputStream before > > > a reset is done). So if you set the keepAlive timeout on TCPClientSOContainer > > > to 60 seconds (60000 ms) a reset on outputStream in Client object only happens > > > after 60000 sent messages... I got a out of memory because of this. > > > I had to subclass TCPClientSOContainer and override getConnectTimeout and > > > createConnection to make it work correctly but it might be better to solve this > > > in TCPClientSOContainer as I guess other users can have the same problem. > > > > > > Reproducible: Always > > > > Hi Franky... > > > > So...the original intention for the keepAlive (on the Client) was for the > > keepAlive interval...so I've fixed the Client/2 constructor to be: > > > > public Client(ISynchAsynchEventHandler handler, int keepAlive) { > > if (handler == null) > > throw new NullPointerException("event handler cannot be null"); > > //$NON-NLS-1$ > > this.handler = handler; > > this.keepAlive = keepAlive; > > containerID = handler.getEventHandlerID(); > > this.properties = new HashMap(); > > } > > > > This should fix the issue you describe here > > > > > Moreover the keepAlive timeout is passed as argument to construct a Client > > > object (in createConnection method) while the argument in Client constructor > > > represents "max messages" (i.e. number of messages sent on outputStream before > > > a reset is done). So if you set the keepAlive timeout on TCPClientSOContainer > > > to 60 seconds (60000 ms) a reset on outputStream in Client object only happens > > > after 60000 sent messages... I got a out of memory because of this. > > > I had to subclass TCPClientSOContainer and override getConnectTimeout and > > > createConnection to make it work correctly but it might be better to solve this > > > in TCPClientSOContainer as I guess other users can have the same problem. > > > > > > Reproducible: Always > > > > Now...WRT the first issue...the connect timeout probably should be another/new > > parameter...and when we go through an API change (addition) I can add > > this...but in immediate term the only way to configure for the moment is what > > you did (override getConnectTimeout()). I understand that this is not > > optimal...but I think perhaps another bug should be opened specifically for > > this issue (connect timeout configuration) and it should be dealt with through > > an API change/addition (a connectTimeout parameter). > > > > So...if you agree with the change to Client/2 as described above, I'll release > > it to master, and then we can open another bug for the connect timeout > > issue...and deal with it during a major version change for o.e.e.provider. > > > > Thanks for reporting these issues. > > Hi Scott, > > Thanks for the quick response. I'm ok with the Client/2 fix. > Concerning the connect timeout: problem is that this connect timeout is at the > end used to override the keepAlive of Client during the connect call. So it > seems to me that the connect timeout and keepAlive are interchangeable. Let me > explain in more detail: > After creating a TCPClientSOContainer instance, we need to call connect method > to connect to a given server (identified in targetID). This connect method is > defined in ClientSOContainer and will: > - call createConnection (method of TCPClientSOContainer) which returns a Client > instance > - call getConnectTimeout (method of TCPClientSOContainer) to get the > connectTimeout --> the default 30 sec > - call connect method on the connection (Client instance) and pass > connectTimeout to this method > - in the connect method of Client instance the passed connectTimeout is used to > override the keepAlive > > So at the end no matter what value you give for keepAlive it always ends with > the default 30 seconds connect timeout defined in TCPClientSOContainer. > > It's important in my case to have keepAlive in Client configurable, so either > connectTimeout on TCPClientSOContainer must be configurable (if keepAlive and > connectTimeout represent the same timeout) or connectTimeout should not be used > to override keepAlive in Client connect method (if they don't represent the > same timeout). > > Best regards, > > Franky Hi Franky, I think what you describe above is simply a bug in connect...i.e. the keepAlive should *not* be overridden by the connect timeout value (as it is in this code from connect: ... try { keepAlive = timeout; final Socket s = fact.createSocket(anURI.getHost(), anURI.getPort(), keepAlive); ... I've now changed this code to be: try { final Socket s = fact.createSocket(anURI.getHost(), anURI.getPort(), timeout); Note that the keepAlive is still subsequently used (in the call to setSocketOptions(s)). So I think that this will give the desired behavior...now keepAlive will be set by the Client/2 constructor...while the *connect timeout* will be passed directly from the connect(ID,Object,int timeout) to the createSocket call...and not override the keepAlive. The keepAlive (if > 0) will then set the socket.setSoTimeout(keepAlive); Does this work for you? I've attached a revised version of Client for your comment...please let me know as soon as you can and if ok I'll release the fix prior to ECF 3.5.3. Created attachment 206157 [details]
Updated version of Client.java
Updated version of Client.java
Sorry for the late reply, I was on holiday and had limited email access. The fix is ok as there's a clear distinction between keepAlive and connect timeout. For the fact that connect timeout is not configurable in TCPClientSOContainer I'll create a separate bug entry. Thanks. Franky (In reply to comment #3) > (In reply to comment #2) > > (In reply to comment #1) > > > (In reply to comment #0) > > > > Build Identifier: ECF 3.5.2 > > > > > > > > While it's possible to provide a keepAlive timeout at creation of ecf generic > > > > client container (TCPClientSOContainer), this keepAlive timeout is not used > > > > because the method getConnectTimeout always returns the default timeout (which > > > > is 30 seconds). This means the keepAlive timeout is not configurable. > > > > Moreover the keepAlive timeout is passed as argument to construct a Client > > > > object (in createConnection method) while the argument in Client constructor > > > > represents "max messages" (i.e. number of messages sent on outputStream before > > > > a reset is done). So if you set the keepAlive timeout on TCPClientSOContainer > > > > to 60 seconds (60000 ms) a reset on outputStream in Client object only happens > > > > after 60000 sent messages... I got a out of memory because of this. > > > > I had to subclass TCPClientSOContainer and override getConnectTimeout and > > > > createConnection to make it work correctly but it might be better to solve this > > > > in TCPClientSOContainer as I guess other users can have the same problem. > > > > > > > > Reproducible: Always > > > > > > Hi Franky... > > > > > > So...the original intention for the keepAlive (on the Client) was for the > > > keepAlive interval...so I've fixed the Client/2 constructor to be: > > > > > > public Client(ISynchAsynchEventHandler handler, int keepAlive) { > > > if (handler == null) > > > throw new NullPointerException("event handler cannot be null"); > > > //$NON-NLS-1$ > > > this.handler = handler; > > > this.keepAlive = keepAlive; > > > containerID = handler.getEventHandlerID(); > > > this.properties = new HashMap(); > > > } > > > > > > This should fix the issue you describe here > > > > > > > Moreover the keepAlive timeout is passed as argument to construct a Client > > > > object (in createConnection method) while the argument in Client constructor > > > > represents "max messages" (i.e. number of messages sent on outputStream before > > > > a reset is done). So if you set the keepAlive timeout on TCPClientSOContainer > > > > to 60 seconds (60000 ms) a reset on outputStream in Client object only happens > > > > after 60000 sent messages... I got a out of memory because of this. > > > > I had to subclass TCPClientSOContainer and override getConnectTimeout and > > > > createConnection to make it work correctly but it might be better to solve this > > > > in TCPClientSOContainer as I guess other users can have the same problem. > > > > > > > > Reproducible: Always > > > > > > Now...WRT the first issue...the connect timeout probably should be another/new > > > parameter...and when we go through an API change (addition) I can add > > > this...but in immediate term the only way to configure for the moment is what > > > you did (override getConnectTimeout()). I understand that this is not > > > optimal...but I think perhaps another bug should be opened specifically for > > > this issue (connect timeout configuration) and it should be dealt with through > > > an API change/addition (a connectTimeout parameter). > > > > > > So...if you agree with the change to Client/2 as described above, I'll release > > > it to master, and then we can open another bug for the connect timeout > > > issue...and deal with it during a major version change for o.e.e.provider. > > > > > > Thanks for reporting these issues. > > > > Hi Scott, > > > > Thanks for the quick response. I'm ok with the Client/2 fix. > > Concerning the connect timeout: problem is that this connect timeout is at the > > end used to override the keepAlive of Client during the connect call. So it > > seems to me that the connect timeout and keepAlive are interchangeable. Let me > > explain in more detail: > > After creating a TCPClientSOContainer instance, we need to call connect method > > to connect to a given server (identified in targetID). This connect method is > > defined in ClientSOContainer and will: > > - call createConnection (method of TCPClientSOContainer) which returns a Client > > instance > > - call getConnectTimeout (method of TCPClientSOContainer) to get the > > connectTimeout --> the default 30 sec > > - call connect method on the connection (Client instance) and pass > > connectTimeout to this method > > - in the connect method of Client instance the passed connectTimeout is used to > > override the keepAlive > > > > So at the end no matter what value you give for keepAlive it always ends with > > the default 30 seconds connect timeout defined in TCPClientSOContainer. > > > > It's important in my case to have keepAlive in Client configurable, so either > > connectTimeout on TCPClientSOContainer must be configurable (if keepAlive and > > connectTimeout represent the same timeout) or connectTimeout should not be used > > to override keepAlive in Client connect method (if they don't represent the > > same timeout). > > > > Best regards, > > > > Franky > > Hi Franky, > > I think what you describe above is simply a bug in connect...i.e. the keepAlive > should *not* be overridden by the connect timeout value (as it is in this code > from connect: > > ... > try { > keepAlive = timeout; > final Socket s = fact.createSocket(anURI.getHost(), anURI.getPort(), > keepAlive); > ... > > I've now changed this code to be: > > try { > final Socket s = fact.createSocket(anURI.getHost(), anURI.getPort(), > timeout); > > Note that the keepAlive is still subsequently used (in the call to > setSocketOptions(s)). > > So I think that this will give the desired behavior...now keepAlive will be set > by the Client/2 constructor...while the *connect timeout* will be passed > directly from the connect(ID,Object,int timeout) to the createSocket call...and > not override the keepAlive. The keepAlive (if > 0) will then set the > socket.setSoTimeout(keepAlive); > > Does this work for you? I've attached a revised version of Client for your > comment...please let me know as soon as you can and if ok I'll release the fix > prior to ECF 3.5.3. Forgot to resolve as fixed. |