Introduced Go module support as v4, removed obsolete CloseNotifier() #1269

alexaandru · 2019-01-30T11:01:51Z

This reintroduces support for Go modules, as v4

CloseNotifier() is removed as it has been obsoleted, see https://golang.org/doc/go1.11#net/http
It was already NOT working (not sending signals) as of 1.11, the functionality was gone already, we merely deleted the functions that exposed it. If everyone still rely on it they should migrate to using c.Request().Context().Done() instead.

Closes #1268, #1255

alexaandru · 2019-01-30T11:02:32Z

@vishr @im-kulikov the branch is ready for review. Cheers!

codecov · 2019-01-30T11:09:58Z

Codecov Report

Merging #1269 into master will increase coverage by 0.25%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master    #1269      +/-   ##
==========================================
+ Coverage   81.63%   81.88%   +0.25%     
==========================================
  Files          26       26              
  Lines        1938     1932       -6     
==========================================
  Hits         1582     1582              
+ Misses        248      242       -6     
  Partials      108      108

Impacted Files	Coverage Δ
middleware/logger.go	`85.18% <ø> (ø)`	⬆️
middleware/csrf.go	`76.71% <ø> (ø)`	⬆️
response.go	`69.44% <ø> (+3.65%)`	⬆️
middleware/key_auth.go	`71.42% <ø> (ø)`	⬆️
middleware/jwt.go	`79.48% <ø> (ø)`	⬆️
echo.go	`87.63% <ø> (ø)`	⬆️
middleware/method_override.go	`84.61% <ø> (ø)`	⬆️
middleware/recover.go	`72.72% <ø> (ø)`	⬆️
middleware/request_id.go	`80% <ø> (ø)`	⬆️
middleware/secure.go	`92% <ø> (ø)`	⬆️
... and 12 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 282a44d...33f2dbe. Read the comment docs.

codecov · 2019-01-30T11:09:59Z

Codecov Report

Merging #1269 into master will increase coverage by 0.25%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master    #1269      +/-   ##
==========================================
+ Coverage   81.63%   81.88%   +0.25%     
==========================================
  Files          26       26              
  Lines        1938     1932       -6     
==========================================
  Hits         1582     1582              
+ Misses        248      242       -6     
  Partials      108      108

Impacted Files	Coverage Δ
middleware/logger.go	`85.18% <ø> (ø)`	⬆️
middleware/csrf.go	`76.71% <ø> (ø)`	⬆️
response.go	`69.44% <ø> (+3.65%)`	⬆️
middleware/static.go	`66.15% <ø> (ø)`	⬆️
middleware/key_auth.go	`71.42% <ø> (ø)`	⬆️
middleware/jwt.go	`79.48% <ø> (ø)`	⬆️
echo.go	`87.63% <ø> (ø)`	⬆️
middleware/method_override.go	`84.61% <ø> (ø)`	⬆️
middleware/recover.go	`72.72% <ø> (ø)`	⬆️
middleware/request_id.go	`80% <ø> (ø)`	⬆️
... and 12 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 282a44d...6d9e043. Read the comment docs.

alexaandru · 2019-01-30T11:13:17Z

Oh, for some weird reason, go.sum is NOT generated on my end. No clue why :-) So I could not commit that one. Technically it's only recommended to include it, but not required (it does not act as a lock file, the go.mod has all the information needed to determine the versions to be used). I'd be happy to include it if only I could figure out why is not created... :-)

im-kulikov · 2019-01-30T11:25:00Z

hm..

go mod tidy
go mod download
go mod verify

And I see go.sum

alexaandru · 2019-01-30T11:56:19Z

Thanks! go mod tidy did it! :-) I committed it now.

im-kulikov · 2019-01-30T13:36:27Z

echo.go:766

func (ln tcpKeepAliveListener) Accept() (c net.Conn, err error) {
	tc, err := ln.AcceptTCP()
	if err != nil {
		return
	}
	tc.SetKeepAlive(true)
	tc.SetKeepAlivePeriod(3 * time.Minute)
	return tc, nil
}

	tc.SetKeepAlive(true)
	tc.SetKeepAlivePeriod(3 * time.Minute)

can return errors, mb replace it with:

func (ln tcpKeepAliveListener) Accept() (c net.Conn, err error) {
	tc, err := ln.AcceptTCP()
	if err != nil {
		return
	}
	if err = tc.SetKeepAlive(true); err != nil {
                return
        }
	if err = tc.SetKeepAlivePeriod(3 * time.Minute); err != nil {
                return
        }
	return tc, nil
}

middleware/compress.go:112 errors ignored

func (w *gzipResponseWriter) Flush() {
	w.Writer.(*gzip.Writer).Flush()
}

im-kulikov

LGTM

alexaandru · 2019-01-30T13:46:57Z

Thanks @im-kulikov ! @vishr are we good?

As for your comment, yes, that change looks good to me! But to keep things clean, please open a separate ticket. Also, FWIW it can be simplified a bit:

func (ln tcpKeepAliveListener) Accept() (c net.Conn, err error) {
	if c, err = ln.AcceptTCP(); err != nil {
		return
	}
	if err = c.SetKeepAlive(true); err != nil {
                return
        }
	if err = c.SetKeepAlivePeriod(3 * time.Minute); err != nil {
                return
        }

	return
}

im-kulikov · 2019-01-30T13:55:52Z

@alexaandru done #1270

alexaandru · 2019-01-30T14:10:42Z

Thank you :) and sorry, looks like we both missed a critical thing... the version :) I'll update mine to 4.0.0 and force push it, and then you'll need to rebase on top of it and call it 4.0.1

…mechanism This reintroduces support for Go modules, as v4. CloseNotifier() is removed as it has been obsoleted, see https://golang.org/doc/go1.11#net/http It was already NOT working (not sending signals) as of 1.11 the functionality was gone, we merely deleted the functions that exposed it. If anyone still relies on it they should migrate to using `c.Request().Context().Done()` instead. Closes #1268, #1255

im-kulikov · 2019-01-30T14:16:17Z

@alexaandru done

alexaandru · 2019-01-30T14:29:03Z

Alright then, I reviewed it one more time, you reviewed it @im-kulikov , CI is happy. I'm merging it :-)

im-kulikov · 2019-01-30T14:37:39Z

my PR was based on your.. I fix it - #1271

alexaandru · 2019-01-30T14:43:54Z

Odd, I know it was based off of that, but the commit did not change (well, after the 4.0.0 change, that is). The merging simply fast forwarded master on top of that commit, but the SHA didn't change. Your PR should've been unaffected. Sorry for the trouble @im-kulikov :(

im-kulikov · 2019-01-30T14:55:48Z

don't worry, I quickly corrected it

alexaandru · 2019-01-30T14:56:54Z

Thanks! :)

vishr · 2019-01-30T15:52:51Z

You guys rock!

alexaandru · 2019-01-30T16:02:43Z

We're just having fun with our favorite web framework ;-) Cheers!

vishr · 2019-01-30T16:04:21Z

@alexaandru On Gitter someone is having problem with v4, can you address that?

This is new and it's a blocker:
package github.com/labstack/echo/v4: cannot find package "github.com/labstack/echo/v4" in any of:
/usr/lib/go-1.10/src/github.com/labstack/echo/v4 (from $GOROOT)
/var/lib/jenkins/jobs/customermanagement/builds/11/src/github.com/labstack/echo/v4 (from $GOPATH)

im-kulikov · 2019-01-30T16:05:48Z

@vishr @alexaandru /usr/lib/go-1.10 mb problem in that?

im-kulikov · 2019-01-30T16:08:14Z

I'm not sure, that suffix would work with less than go1.11

alexaandru · 2019-01-30T16:20:33Z

@vishr @im-kulikov the Go requirements I took straight from Go wiki: https://github.com/golang/go/wiki/Modules#releasing-modules-v2-or-higher I really think they would be the ones to know best which Go version supports what... If they have a Go 1.10 version below 1.10.3, then yes, it will not work. If however they have at least 1.10.3, then it should work.

They should either upgrade their Go OR stick to Echo v3.3.10. Removing support for Go modules should really be our last option (I hope)... Thoughts?

vishr · 2019-01-30T16:21:35Z

@vishr @im-kulikov the Go requirements I took straight from Go wiki: https://github.com/golang/go/wiki/Modules#releasing-modules-v2-or-higher I really think they would be the ones to know best which Go version supports what... If they have a Go 1.10 version below 1.10.3, then yes, it will not work. If however they have at least 1.10.3, then it should work.

They should either upgrade their Go OR stick to Echo v3.3.10. Removing support for Go modules should really be our last option (I hope)... Thoughts?

I agree, the guy is using v3.3.9 now - which works.

alexaandru · 2019-01-30T16:21:35Z

Can we at least get the go version that they're using?

vishr · 2019-01-30T16:22:15Z

Can we at least get the go version that they're using?

From the logs it seems Go 1.10.x

alexaandru · 2019-01-30T16:22:36Z

Well, it's that tiny x that matters the most in this case :-)

vishr · 2019-01-30T16:23:07Z

Well, it's that tiny x that matters the most in this case :-)

Not sure.

vishr · 2019-01-30T16:23:36Z

It is ok until someone else complains ;)

alexaandru · 2019-01-30T16:30:52Z

In all fairness, the only ones complaining are the ones that have the least reasons: the ones that do not use any dependency management at all. We did release a MAJOR version. Major versions break things. Everywhere, not just in our project. If someone does not like their project to be broken, they should use a dependency management mechanism. Or read the Release Notes... Just sayin'...

alexaandru force-pushed the v4-preview branch from 6add227 to 33f2dbe Compare January 30, 2019 11:09

alexaandru force-pushed the v4-preview branch from 557884d to 905c63a Compare January 30, 2019 11:56

im-kulikov approved these changes Jan 30, 2019

View reviewed changes

alexaandru force-pushed the v4-preview branch from 905c63a to 6d9e043 Compare January 30, 2019 14:13

alexaandru merged commit 6d9e043 into master Jan 30, 2019

alexaandru deleted the v4-preview branch January 30, 2019 14:30

alexaandru mentioned this pull request Jan 30, 2019

"http.CloseNotifier" is deprecated #1255

Closed

siredwin mentioned this pull request Jan 12, 2020

obsolete CloseNotifier() alexandrevicenzi/go-sse#14

Closed

externaljdev mentioned this pull request Jul 15, 2021

CloseNotify() incompatible with the Labstack Echo v4: r3labs/sse#92

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Introduced Go module support as v4, removed obsolete CloseNotifier() #1269

Introduced Go module support as v4, removed obsolete CloseNotifier() #1269

alexaandru commented Jan 30, 2019

alexaandru commented Jan 30, 2019

codecov bot commented Jan 30, 2019

codecov bot commented Jan 30, 2019 •

edited

Loading

alexaandru commented Jan 30, 2019

im-kulikov commented Jan 30, 2019

alexaandru commented Jan 30, 2019

im-kulikov commented Jan 30, 2019 •

edited

Loading

im-kulikov left a comment

alexaandru commented Jan 30, 2019

im-kulikov commented Jan 30, 2019

alexaandru commented Jan 30, 2019

im-kulikov commented Jan 30, 2019

alexaandru commented Jan 30, 2019

im-kulikov commented Jan 30, 2019

alexaandru commented Jan 30, 2019

im-kulikov commented Jan 30, 2019

alexaandru commented Jan 30, 2019

vishr commented Jan 30, 2019

alexaandru commented Jan 30, 2019

vishr commented Jan 30, 2019

im-kulikov commented Jan 30, 2019 •

edited

Loading

im-kulikov commented Jan 30, 2019

alexaandru commented Jan 30, 2019

vishr commented Jan 30, 2019 •

edited

Loading

alexaandru commented Jan 30, 2019

vishr commented Jan 30, 2019 •

edited

Loading

alexaandru commented Jan 30, 2019

vishr commented Jan 30, 2019

vishr commented Jan 30, 2019

alexaandru commented Jan 30, 2019

Introduced Go module support as v4, removed obsolete CloseNotifier() #1269

Introduced Go module support as v4, removed obsolete CloseNotifier() #1269

Conversation

alexaandru commented Jan 30, 2019

alexaandru commented Jan 30, 2019

codecov bot commented Jan 30, 2019

Codecov Report

codecov bot commented Jan 30, 2019 • edited Loading

Codecov Report

alexaandru commented Jan 30, 2019

im-kulikov commented Jan 30, 2019

alexaandru commented Jan 30, 2019

im-kulikov commented Jan 30, 2019 • edited Loading

im-kulikov left a comment

Choose a reason for hiding this comment

alexaandru commented Jan 30, 2019

im-kulikov commented Jan 30, 2019

alexaandru commented Jan 30, 2019

im-kulikov commented Jan 30, 2019

alexaandru commented Jan 30, 2019

im-kulikov commented Jan 30, 2019

alexaandru commented Jan 30, 2019

im-kulikov commented Jan 30, 2019

alexaandru commented Jan 30, 2019

vishr commented Jan 30, 2019

alexaandru commented Jan 30, 2019

vishr commented Jan 30, 2019

im-kulikov commented Jan 30, 2019 • edited Loading

im-kulikov commented Jan 30, 2019

alexaandru commented Jan 30, 2019

vishr commented Jan 30, 2019 • edited Loading

alexaandru commented Jan 30, 2019

vishr commented Jan 30, 2019 • edited Loading

alexaandru commented Jan 30, 2019

vishr commented Jan 30, 2019

vishr commented Jan 30, 2019

alexaandru commented Jan 30, 2019

codecov bot commented Jan 30, 2019 •

edited

Loading

im-kulikov commented Jan 30, 2019 •

edited

Loading

im-kulikov commented Jan 30, 2019 •

edited

Loading

vishr commented Jan 30, 2019 •

edited

Loading

vishr commented Jan 30, 2019 •

edited

Loading