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

Is this a potential bug of SetWriteDeadline? #38

Closed
itomsawyer opened this issue Nov 5, 2017 · 3 comments
Closed

Is this a potential bug of SetWriteDeadline? #38

itomsawyer opened this issue Nov 5, 2017 · 3 comments

Comments

@itomsawyer
Copy link
Contributor

Hi , I have read the source code of stompngo and find something hard to understand.

Here's a reader go routine which can be set read timeout by conn.ReadDealLine

func (c *Connection) setReadDeadline() {
	if c.dld.rde && c.dld.rds {
		_ = c.netconn.SetReadDeadline(time.Now().Add(c.dld.rdld))
	}
}

The reader use c.dld.rde && c.dld.rds to check if read deadline will be set.
Buf a writer use c.dld.wde && c.dld.dns to check if write deadline will be set.
Should it use c.dld.wde && c.dld.wds instead ?

func (c *Connection) wireWrite(d wiredata) {
	f := &d.frame
	// fmt.Printf("WWD01 f:[%v]\n", f)
	switch f.Command {
	case "\n": // HeartBeat frame
		if c.dld.wde && c.dld.dns {
			_ = c.netconn.SetWriteDeadline(time.Now().Add(c.dld.wdld))
		}

Looking forward to your reply

@gmallard
Copy link
Owner

gmallard commented Nov 5, 2017

Agreed, it is bug.

Repeated numerous times in writer.go.

If you give me a PR, I will accept it.

Watch line 242, it requires some extra care.

@itomsawyer
Copy link
Contributor Author

@gmallard new pull request has been submitted, plz check it out

@gmallard
Copy link
Owner

gmallard commented Mar 8, 2018

Closing this per:

cc91367

cc91367

@gmallard gmallard closed this as completed Mar 8, 2018
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

No branches or pull requests

2 participants